Re: [PATCH] Documentation/Makefile: fix lint-docs mkdir dependency
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Oct 28, 2021, 18:30 UTC
- Message-ID
- <211028.86o879vvtp.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <YXrYh9aPA8csq+UQ@coredump.intra.peff.net>
On Thu, Oct 28 2021, Jeff King wrote:
Show 26 quoted lines
> On Thu, Oct 28, 2021 at 09:45:14AM -0700, Junio C Hamano wrote: > >> Jeff King <peff@peff.net> writes: >> >> > There's an in-between, I'd think, where the many "foo/bar/baz/$@" >> > targets have an order-dependency on "foo/bar/baz", and that single rule >> > uses "mkdir -p" to create all of the directories. >> > >> > It doesn't buy us much simplification in this case, though, because >> > various rules independently depend on .build/gitlink/lint-docs/howto, >> > .built/gitlink/lint-docs, and .build/gitlink, etc. So we still end up >> > with roughly the same number of rules, though the directory rules don't >> > have to depend on one another. >> > >> > It also means that these "mkdir -p" may race with each other, though in >> > general I'd hope that most "mkdir" implements could handle this. >> > >> > Something like this works, I think: >> >> Hmph, what I actually meant was to make sure that the recipe to >> create the files to have "mkdir -p $(basename $@)" in front, instead >> of having "we need to prepare the containing directory in order to >> have a file there" in the makefile. > > Yeah, I agree that's simpler, and is what Ævar showed. But it is slower, > because we run a bunch of redundant "mkdir -p" calls, one per file.
Here's a method that's both less verbose in lines & also faster, but maybe too clever a use of GNU make features, on top of "next".
It relies on $(wildcard) returning an empty list on a non-existing filename, and then on $(if) to either expand to "mkdir -p $(@D)", or nothing, and (perhaps in an ugly way?) piggy-backs on an existing $@ rule, so one rule has two $(QUIET_*) uses.
With:
HEAD~2 = next
HEAD~1 = unconditional mkdir -p, upthread <211028.861r45y3pt.gmgdl@evledraar.gmail.com>
HEAD = the below patchWe get these results, i.e. it's also faster:
$ hyperfine --warmup 5 -m 30 -L s ",~,~2" -p 'git checkout HEAD{s} -- Makefile; rm -rf .build' 'make -j8 lint-docs R={s}' -c 'git checkout HEAD -- Makefile'
Benchmark #1: make -j8 lint-docs R=
Time (mean ± σ): 628.5 ms ± 43.2 ms [User: 2.385 s, System: 0.445 s]
Range (min … max): 605.6 ms … 787.7 ms 30 runs
Benchmark #2: make -j8 lint-docs R=~
Time (mean ± σ): 770.2 ms ± 12.7 ms [User: 3.446 s, System: 0.666 s]
Range (min … max): 756.3 ms … 831.4 ms 30 runs
Benchmark #3: make -j8 lint-docs R=~2
Time (mean ± σ): 696.9 ms ± 3.5 ms [User: 2.967 s, System: 0.600 s]
Range (min … max): 691.2 ms … 706.2 ms 30 runs
Summary
'make -j8 lint-docs R=' ran
1.11 ± 0.08 times faster than 'make -j8 lint-docs R=~2'
1.23 ± 0.09 times faster than 'make -j8 lint-docs R=~'The one negative thing is that we'll return an inconsistent set of "mkdir" lines, since it's racy, but here we're using the race to our benefit. It doesn't matter for the end result if we e.g. created a more nested directory first, or needed two "mkdir -p" calls because we did a shallower one first.
Do you think it's worth submitting that as a non-throwaway?
diff --git a/Documentation/Makefile b/Documentation/Makefile index ed656db2ae9..99c2f9d02cf 100644 --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -234,6 +234,7 @@ ifndef V QUIET_DBLATEX = @echo ' ' DBLATEX $@; QUIET_XSLTPROC = @echo ' ' XSLTPROC $@; QUIET_GEN = @echo ' ' GEN $@; + QUIET_MKDIR_P = @echo ' ' MKDIR -p $(@D); QUIET_STDERR = 2> /dev/null QUIET_SUBDIR0 = +@subdir= QUIET_SUBDIR1 = ;$(NO_SUBDIR) echo ' ' SUBDIR $$subdir; \ @@ -463,25 +464,10 @@ quick-install-html: require-htmlrepo print-man1: @for i in $(MAN1_TXT); do echo $$i; done -## Lint: Common -.build: - $(QUIET)mkdir $@ -.build/lint-docs: | .build - $(QUIET)mkdir $@ - -## Lint: gitlink -.build/lint-docs/gitlink: | .build/lint-docs - $(QUIET)mkdir $@ -.build/lint-docs/gitlink/howto: | .build/lint-docs/gitlink - $(QUIET)mkdir $@ -.build/lint-docs/gitlink/config: | .build/lint-docs/gitlink - $(QUIET)mkdir $@ LINT_DOCS_GITLINK = $(patsubst %.txt,.build/lint-docs/gitlink/%.ok,$(HOWTO_TXT) $(DOC_DEP_TXT)) -$(LINT_DOCS_GITLINK): | .build/lint-docs/gitlink -$(LINT_DOCS_GITLINK): | .build/lint-docs/gitlink/howto -$(LINT_DOCS_GITLINK): | .build/lint-docs/gitlink/config $(LINT_DOCS_GITLINK): lint-gitlink.perl $(LINT_DOCS_GITLINK): .build/lint-docs/gitlink/%.ok: %.txt + $(if $(wildcard $(@D)),,$(QUIET_MKDIR_P)$(shell mkdir -p $(@D))) $(QUIET_LINT_GITLINK)$(PERL_PATH) lint-gitlink.perl \ $< \ $(HOWTO_TXT) $(DOC_DEP_TXT) \ @@ -492,23 +478,19 @@ $(LINT_DOCS_GITLINK): .build/lint-docs/gitlink/%.ok: %.txt lint-docs-gitlink: $(LINT_DOCS_GITLINK) ## Lint: man-end-blurb -.build/lint-docs/man-end-blurb: | .build/lint-docs - $(QUIET)mkdir $@ LINT_DOCS_MAN_END_BLURB = $(patsubst %.txt,.build/lint-docs/man-end-blurb/%.ok,$(MAN_TXT)) -$(LINT_DOCS_MAN_END_BLURB): | .build/lint-docs/man-end-blurb $(LINT_DOCS_MAN_END_BLURB): lint-man-end-blurb.perl $(LINT_DOCS_MAN_END_BLURB): .build/lint-docs/man-end-blurb/%.ok: %.txt - $(QUIET_LINT_MANEND)$(PERL_PATH) lint-man-end-blurb.perl $< >$@ + $(if $(wildcard $(@D)),,$(QUIET_MKDIR_P)$(shell mkdir -p $(@D))) + $(QUIET_LINT_MANEND)$($(PERL_PATH) lint-man-end-blurb.perl $< >$@ .PHONY: lint-docs-man-end-blurb lint-docs-man-end-blurb: $(LINT_DOCS_MAN_END_BLURB) ## Lint: man-section-order -.build/lint-docs/man-section-order: | .build/lint-docs - $(QUIET)mkdir $@ LINT_DOCS_MAN_SECTION_ORDER = $(patsubst %.txt,.build/lint-docs/man-section-order/%.ok,$(MAN_TXT)) -$(LINT_DOCS_MAN_SECTION_ORDER): | .build/lint-docs/man-section-order $(LINT_DOCS_MAN_SECTION_ORDER): lint-man-section-order.perl $(LINT_DOCS_MAN_SECTION_ORDER): .build/lint-docs/man-section-order/%.ok: %.txt + $(if $(wildcard $(@D)),,$(QUIET_MKDIR_P)$(shell mkdir -p $(@D))) $(QUIET_LINT_MANSEC)$(PERL_PATH) lint-man-section-order.perl $< >$@ .PHONY: lint-docs-man-section-order lint-docs-man-section-order: $(LINT_DOCS_MAN_SECTION_ORDER)