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

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

[Ramsay Jones]

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

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

[Junio C Hamano]

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.

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

[Ramsay Jones]

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

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

[Jeff King]

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).

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

[Ramsay Jones]

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

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

[Jeff King]

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

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

[Junio C Hamano]

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.