* [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE @ 2020-11-05 21:03 Ramsay Jones 2020-11-05 21:52 ` Junio C Hamano 2020-11-06 18:18 ` Jeff King 0 siblings, 2 replies; 7+ messages in thread From: Ramsay Jones @ 2020-11-05 21:03 UTC (permalink / raw) To: GIT Mailing-list; +Cc: Junio C Hamano The 'clean' target is still noticeably slow on cygwin, despite the substantial improvement made by the previous patch. For example, the second invocation of 'make clean' below: $ make clean >/dev/null 2>&1 $ make clean ... make[1]: Entering directory '/home/ramsay/git/Documentation' make[2]: Entering directory '/home/ramsay/git' make[2]: 'GIT-VERSION-FILE' is up to date. make[2]: Leaving directory '/home/ramsay/git' ... $ has been timed at 12.364s on my laptop (on old core i5-4200M @ 2.50GHz, 8GB RAM, 1TB HDD). Notice that the 'clean' target is making a nested call to the parent Makefile to ensure that the GIT-VERSION-FILE is up-to-date (prior to the previous patch, there would have been _two_ such invocations). This is to ensure that the $(GIT_VERSION) make variable is set, once that file had been included. However, the 'clean' target does not use the $(GIT_VERSION) variable, so this is wasted effort. In order to eliminate such wasted effort, use the value of the internal $(MAKECMDGOALS) variable to only '-include ../GIT-VERSION-FILE' when the target is not 'clean'. (This drops the time down to 10.361s, on my laptop, giving an improvement of 16.20%). Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> --- Documentation/Makefile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/Makefile b/Documentation/Makefile index 652d57a1b6..5c680024eb 100644 --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -272,7 +272,9 @@ install-html: html ../GIT-VERSION-FILE: FORCE $(QUIET_SUBDIR0)../ $(QUIET_SUBDIR1) GIT-VERSION-FILE +ifneq ($(MAKECMDGOALS),clean) -include ../GIT-VERSION-FILE +endif # # Determine "include::" file references in asciidoc files. -- 2.29.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE 2020-11-05 21:03 [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE Ramsay Jones @ 2020-11-05 21:52 ` Junio C Hamano 2020-11-06 1:30 ` Ramsay Jones 2020-11-06 18:18 ` Jeff King 1 sibling, 1 reply; 7+ messages in thread From: Junio C Hamano @ 2020-11-05 21:52 UTC (permalink / raw) To: Ramsay Jones; +Cc: GIT Mailing-list Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > The 'clean' target is still noticeably slow on cygwin, despite the > substantial improvement made by the previous patch. For example, the > second invocation of 'make clean' below: > > $ make clean >/dev/null 2>&1 > $ make clean > ... > make[1]: Entering directory '/home/ramsay/git/Documentation' > make[2]: Entering directory '/home/ramsay/git' > make[2]: 'GIT-VERSION-FILE' is up to date. > make[2]: Leaving directory '/home/ramsay/git' > ... > $ > > has been timed at 12.364s on my laptop (on old core i5-4200M @ 2.50GHz, > 8GB RAM, 1TB HDD). > > Notice that the 'clean' target is making a nested call to the parent > Makefile to ensure that the GIT-VERSION-FILE is up-to-date (prior to > the previous patch, there would have been _two_ such invocations). > This is to ensure that the $(GIT_VERSION) make variable is set, once > that file had been included. However, the 'clean' target does not use > the $(GIT_VERSION) variable, so this is wasted effort. > > In order to eliminate such wasted effort, use the value of the internal > $(MAKECMDGOALS) variable to only '-include ../GIT-VERSION-FILE' when the > target is not 'clean'. (This drops the time down to 10.361s, on my laptop, > giving an improvement of 16.20%). This obviously relies on the fact that none of our build products to be cleaned are named using $(GIT_VERSION)---in other words, if our cleaning rule contained rm -f git-$(GIT_VERSION)-manual.html this optimization would not work well. Luckily, I think we use GIT_VERSION only to engrave version number in the resulting document, and it does not affect the name of the build product, so this change is safe, I think. > Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> > --- > Documentation/Makefile | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/Makefile b/Documentation/Makefile > index 652d57a1b6..5c680024eb 100644 > --- a/Documentation/Makefile > +++ b/Documentation/Makefile > @@ -272,7 +272,9 @@ install-html: html > ../GIT-VERSION-FILE: FORCE > $(QUIET_SUBDIR0)../ $(QUIET_SUBDIR1) GIT-VERSION-FILE > > +ifneq ($(MAKECMDGOALS),clean) > -include ../GIT-VERSION-FILE > +endif > > # > # Determine "include::" file references in asciidoc files. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE 2020-11-05 21:52 ` Junio C Hamano @ 2020-11-06 1:30 ` Ramsay Jones 0 siblings, 0 replies; 7+ messages in thread From: Ramsay Jones @ 2020-11-06 1:30 UTC (permalink / raw) To: Junio C Hamano; +Cc: GIT Mailing-list On 05/11/2020 21:52, Junio C Hamano wrote: > Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > >> The 'clean' target is still noticeably slow on cygwin, despite the >> substantial improvement made by the previous patch. For example, the >> second invocation of 'make clean' below: >> >> $ make clean >/dev/null 2>&1 >> $ make clean >> ... >> make[1]: Entering directory '/home/ramsay/git/Documentation' >> make[2]: Entering directory '/home/ramsay/git' >> make[2]: 'GIT-VERSION-FILE' is up to date. >> make[2]: Leaving directory '/home/ramsay/git' >> ... >> $ >> >> has been timed at 12.364s on my laptop (on old core i5-4200M @ 2.50GHz, >> 8GB RAM, 1TB HDD). >> >> Notice that the 'clean' target is making a nested call to the parent >> Makefile to ensure that the GIT-VERSION-FILE is up-to-date (prior to >> the previous patch, there would have been _two_ such invocations). >> This is to ensure that the $(GIT_VERSION) make variable is set, once >> that file had been included. However, the 'clean' target does not use >> the $(GIT_VERSION) variable, so this is wasted effort. >> >> In order to eliminate such wasted effort, use the value of the internal >> $(MAKECMDGOALS) variable to only '-include ../GIT-VERSION-FILE' when the >> target is not 'clean'. (This drops the time down to 10.361s, on my laptop, >> giving an improvement of 16.20%). > > This obviously relies on the fact that none of our build products to > be cleaned are named using $(GIT_VERSION)---in other words, if our > cleaning rule contained > > rm -f git-$(GIT_VERSION)-manual.html > > this optimization would not work well. Yes, indeed. > Luckily, I think we use GIT_VERSION only to engrave version number > in the resulting document, and it does not affect the name of the > build product, so this change is safe, I think. Yes, as I said in the commit message above: "However, the 'clean' target does not use the $(GIT_VERSION) variable, so this is wasted effort." Thanks. ATB, Ramsay Jones ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE 2020-11-05 21:03 [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE Ramsay Jones 2020-11-05 21:52 ` Junio C Hamano @ 2020-11-06 18:18 ` Jeff King 2020-11-06 19:38 ` Ramsay Jones 1 sibling, 1 reply; 7+ messages in thread From: Jeff King @ 2020-11-06 18:18 UTC (permalink / raw) To: Ramsay Jones; +Cc: GIT Mailing-list, Junio C Hamano On Thu, Nov 05, 2020 at 09:03:49PM +0000, Ramsay Jones wrote: > diff --git a/Documentation/Makefile b/Documentation/Makefile > index 652d57a1b6..5c680024eb 100644 > --- a/Documentation/Makefile > +++ b/Documentation/Makefile > @@ -272,7 +272,9 @@ install-html: html > ../GIT-VERSION-FILE: FORCE > $(QUIET_SUBDIR0)../ $(QUIET_SUBDIR1) GIT-VERSION-FILE > > +ifneq ($(MAKECMDGOALS),clean) > -include ../GIT-VERSION-FILE > +endif Calling out "clean" here specially feels somewhat backwards to me, in terms of Makefile design. In an ideal world we provide all of the dependencies to "make", and based on the targets we are building, it decides what needs to be done. This works with normal targets, obviously, but also with variables. If we do: FOO = $(shell do-the-thing) then we execute that command only when $(FOO) is needed[1]. But "include" here is tricky. It is loaded regardless of whether the values it contains are needed or not. I wonder if we could do better by giving make more information about what we're expecting to get from it. I.e., if we wrote: GIT_VERSION = $(shell awk '{print $3}' GIT-VERSION-FILE) Then "make clean", not needing the value of $(GIT_VERSION), wouldn't run that shell snippet at all. Of course there's a catch; we are also relying on the include to trigger the dependency. So it is really more like: GIT_VERSION = $(shell make GIT-VERSION-FILE && awk '{print $3}' GIT-VERSION-FILE) I'm not sure how bad that is. Re-invoking make seems like it could get expensive, especially for the common case that we're building actual binaries and we _do_ need the version. But we could probably cut "make" out of the loop entirely. Generating GIT-VERSION-FILE is already a FORCE target, so really: GIT_VERSION = $(shell ./GIT-VERSION-GEN) would be equivalent-ish (with some output changes, and possibly we'd want to stash the value in a file for any other scripts to make use of). This is all just stuff I've written in my editor and not tried. I won't be surprised if there are some gotchas. But it at least seems like a conceptually cleaner path. -Peff [1] Variable assignment actually has a slight problem in the opposite direction: it wants to run the shell snippet every time the variable is referenced. There's a trick to get around that described in 0573831950 (Makefile: avoid running curl-config unnecessarily, 2020-04-04). It's built around evals. In fact, I suspect you could build a function around eval that actually works similar to include, but lazy-loads the file only when one of its stubs is referenced. I.e., something like: GIT_VERSION = $(eval include GIT-VERSION-FILE) would probably work (and for other includes, multiple variables could mention the same file; as soon as it gets included, it overwrites the stubs). ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE 2020-11-06 18:18 ` Jeff King @ 2020-11-06 19:38 ` Ramsay Jones 2020-11-06 20:56 ` Jeff King 2020-11-06 21:43 ` Junio C Hamano 0 siblings, 2 replies; 7+ messages in thread From: Ramsay Jones @ 2020-11-06 19:38 UTC (permalink / raw) To: Jeff King; +Cc: GIT Mailing-list, Junio C Hamano On 06/11/2020 18:18, Jeff King wrote: > On Thu, Nov 05, 2020 at 09:03:49PM +0000, Ramsay Jones wrote: > >> diff --git a/Documentation/Makefile b/Documentation/Makefile >> index 652d57a1b6..5c680024eb 100644 >> --- a/Documentation/Makefile >> +++ b/Documentation/Makefile >> @@ -272,7 +272,9 @@ install-html: html >> ../GIT-VERSION-FILE: FORCE >> $(QUIET_SUBDIR0)../ $(QUIET_SUBDIR1) GIT-VERSION-FILE >> >> +ifneq ($(MAKECMDGOALS),clean) >> -include ../GIT-VERSION-FILE >> +endif > > Calling out "clean" here specially feels somewhat backwards to me, in > terms of Makefile design. In an ideal world we provide all of the > dependencies to "make", and based on the targets we are building, it > decides what needs to be done. > > This works with normal targets, obviously, but also with variables. If > we do: > > FOO = $(shell do-the-thing) > > then we execute that command only when $(FOO) is needed[1]. > > But "include" here is tricky. It is loaded regardless of whether the > values it contains are needed or not. I wonder if we could do better by > giving make more information about what we're expecting to get from it. > I.e., if we wrote: > > GIT_VERSION = $(shell awk '{print $3}' GIT-VERSION-FILE) > > Then "make clean", not needing the value of $(GIT_VERSION), wouldn't run > that shell snippet at all. Of course there's a catch; we are also > relying on the include to trigger the dependency. So it is really more > like: > > GIT_VERSION = $(shell make GIT-VERSION-FILE && awk '{print $3}' GIT-VERSION-FILE) > > I'm not sure how bad that is. Re-invoking make seems like it could get > expensive, especially for the common case that we're building actual > binaries and we _do_ need the version. But we could probably cut "make" > out of the loop entirely. Generating GIT-VERSION-FILE is already a FORCE > target, so really: > > GIT_VERSION = $(shell ./GIT-VERSION-GEN) > > would be equivalent-ish (with some output changes, and possibly we'd > want to stash the value in a file for any other scripts to make use of). > > This is all just stuff I've written in my editor and not tried. I won't > be surprised if there are some gotchas. But it at least seems like a > conceptually cleaner path. > > -Peff > > [1] Variable assignment actually has a slight problem in the opposite > direction: it wants to run the shell snippet every time the variable > is referenced. There's a trick to get around that described in > 0573831950 (Makefile: avoid running curl-config unnecessarily, > 2020-04-04). > > It's built around evals. In fact, I suspect you could build a > function around eval that actually works similar to include, but > lazy-loads the file only when one of its stubs is referenced. I.e., > something like: > > GIT_VERSION = $(eval include GIT-VERSION-FILE) > > would probably work (and for other includes, multiple variables > could mention the same file; as soon as it gets included, it > overwrites the stubs). > Heh, in another reply in this thread, I mentioned that I had an alternate patch I was toying with. If I tell you it was inspired by your commit 0573831950 (Makefile: avoid running curl-config unnecessarily, 04-04-2020), you would probably not be surprised that it looks similar to what you outline here. I had only just started looking at this approach, but it has some wrinkles, so I thought I would look at it after submitting this series 'because this is an easy win'! ;-) I hadn't done so yet, but I had planned to modify the GIT-VERSION-GEN script (with -v parameter, say) to just output the version to stdout, because it would save a sed invocation. It currently looks something like this: diff --git a/Makefile b/Makefile index 372139f1f2..f166fbe067 100644 --- a/Makefile +++ b/Makefile @@ -493,7 +493,11 @@ all:: GIT-VERSION-FILE: FORCE @$(SHELL_PATH) ./GIT-VERSION-GEN --include GIT-VERSION-FILE + +ifeq ($(wildcard GIT-VERSION-FILE),) +$(shell ./GIT-VERSION-GEN) +endif +GIT_VERSION = $(eval GIT_VERSION := $$(shell cat GIT-VERSION-FILE | sed -e 's/^GIT_VERSION = //'))$(GIT_VERSION) # Set our default configuration. # Ignore the 'ifeq' for now (I had several versions, including getting rid of the GIT-VERSION-FILE rule, but that caused problems without changing the Documentation/Makefile, and several others ... (including in contrib)). So, I haven't worked everything out yet, but I had planned to look at this next. ATB, Ramsay Jones ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE 2020-11-06 19:38 ` Ramsay Jones @ 2020-11-06 20:56 ` Jeff King 2020-11-06 21:43 ` Junio C Hamano 1 sibling, 0 replies; 7+ messages in thread From: Jeff King @ 2020-11-06 20:56 UTC (permalink / raw) To: Ramsay Jones; +Cc: GIT Mailing-list, Junio C Hamano On Fri, Nov 06, 2020 at 07:38:06PM +0000, Ramsay Jones wrote: > I hadn't done so yet, but I had planned to modify the GIT-VERSION-GEN script > (with -v parameter, say) to just output the version to stdout, because it > would save a sed invocation. It currently looks something like this: > > diff --git a/Makefile b/Makefile > index 372139f1f2..f166fbe067 100644 > --- a/Makefile > +++ b/Makefile > @@ -493,7 +493,11 @@ all:: > > GIT-VERSION-FILE: FORCE > @$(SHELL_PATH) ./GIT-VERSION-GEN > --include GIT-VERSION-FILE > + > +ifeq ($(wildcard GIT-VERSION-FILE),) > +$(shell ./GIT-VERSION-GEN) > +endif > +GIT_VERSION = $(eval GIT_VERSION := $$(shell cat GIT-VERSION-FILE | sed -e 's/^GIT_VERSION = //'))$(GIT_VERSION) Yeah, not too surprising. If we grow a lot of these fill-in-the-value stubs, it might be worth abstracting it out into a function (using $(call) should be OK, as even Apple's ancient version of GNU make has it). I do think the one that evals an "include" might end up being more readable and flexible, though. I'm not sure if the include needs to be more aggressive about using ":=" though (in a simple test it didn't seem to be, and since we'll be filling in a concrete value that's OK to evaluate multiple times, I think it would be OK). > Ignore the 'ifeq' for now (I had several versions, including getting rid > of the GIT-VERSION-FILE rule, but that caused problems without changing > the Documentation/Makefile, and several others ... (including in contrib)). Yeah, I didn't look at all at what complications we might get from other Makefiles. Though if everything is literally running `GIT-VERSION-GEN` directly then I think they could all use the same code. > So, I haven't worked everything out yet, but I had planned to look at > this next. Sounds good. I mostly wanted to plant the seed in your head. Finding that it was already growing is better still. :) -Peff ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE 2020-11-06 19:38 ` Ramsay Jones 2020-11-06 20:56 ` Jeff King @ 2020-11-06 21:43 ` Junio C Hamano 1 sibling, 0 replies; 7+ messages in thread From: Junio C Hamano @ 2020-11-06 21:43 UTC (permalink / raw) To: Ramsay Jones; +Cc: Jeff King, GIT Mailing-list Ramsay Jones <ramsay@ramsayjones.plus.com> writes: >> It's built around evals. In fact, I suspect you could build a >> function around eval that actually works similar to include, but >> lazy-loads the file only when one of its stubs is referenced. I.e., >> something like: >> >> GIT_VERSION = $(eval include GIT-VERSION-FILE) >> >> would probably work (and for other includes, multiple variables >> could mention the same file; as soon as it gets included, it >> overwrites the stubs). >> > > Heh, in another reply in this thread, I mentioned that I had an alternate > patch I was toying with. Yup, I was wondering if you're going to present that variant, which I was more curious about. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-11-06 21:43 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-05 21:03 [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE Ramsay Jones 2020-11-05 21:52 ` Junio C Hamano 2020-11-06 1:30 ` Ramsay Jones 2020-11-06 18:18 ` Jeff King 2020-11-06 19:38 ` Ramsay Jones 2020-11-06 20:56 ` Jeff King 2020-11-06 21:43 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).