threads / patch / 54581

patch, 8 partsDocumentation/Makefile: conditionally include ../GIT-VERSION-FILE

Subject: [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE

## tl;dr

7 messages between Nov 5, 2020 and Nov 6, 2020. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Ramsay Jones· Nov 5, 2020, 21:03 UTC · lore

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(+)
Show changes to Documentation/Makefile +2 −0
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
Junio C Hamano· Nov 5, 2020, 21:52 UTC · re: Ramsay Jones · lore

Re: [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
Show 28 quoted lines
> 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.

Show 19 quoted lines
> 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.
Ramsay Jones· Nov 6, 2020, 01:30 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE

On 05/11/2020 21:52, Junio C Hamano wrote:
Show 38 quoted lines
> 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

Jeff King· Nov 6, 2020, 18:18 UTC · re: Ramsay Jones · lore

Re: [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE

On Thu, Nov 05, 2020 at 09:03:49PM +0000, Ramsay Jones wrote:
Show 11 quoted lines
> 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).
Ramsay Jones· Nov 6, 2020, 19:38 UTC · re: Jeff King · lore

Re: [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE

On 06/11/2020 18:18, Jeff King wrote:
Show 74 quoted lines
> 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

Jeff King· Nov 6, 2020, 20:56 UTC · re: Ramsay Jones · lore

Re: [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE

On Fri, Nov 06, 2020 at 07:38:06PM +0000, Ramsay Jones wrote:
Show 18 quoted lines
> 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
Junio C Hamano· Nov 6, 2020, 21:43 UTC · re: Ramsay Jones · lore

Re: [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
Show 14 quoted lines
>>     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.

← back to recent threads