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

From: Ramsay Jones <ramsay@ramsayjones.plus.com>
Date: Nov 6, 2020, 01:30 UTC
Message-ID: <2f55ac8d-bcbc-5b4b-a790-4a749b369db0@ramsayjones.plus.com>
In-Reply-To: <xmqqy2jfh3nz.fsf@gitster.c.googlers.com>

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