Re: [PATCH v6 27/39] try_merge_strategy(): use a statically-allocated lock_file object

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 30, 2014, 18:08 UTC
Message-ID: <xmqqiok5rnf4.fsf@gitster.dls.corp.google.com>
In-Reply-To: <542AB853.5000303@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 30 quoted lines

> On 09/26/2014 09:00 PM, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> 
>>> Even the one lockfile object needn't be allocated each time the
>>> function is called.  Instead, define one statically-allocated
>>> lock_file object and reuse it for every call.
>>>
>>> Suggested-by: Jeff King <peff@peff.net>
>>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>>> ---
>>> ...
>>> -	hold_locked_index(lock, 1);
>>> +	hold_locked_index(&lock, 1);
>>>  	refresh_cache(REFRESH_QUIET);
>>>  	if (active_cache_changed &&
>>> -	    write_locked_index(&the_index, lock, COMMIT_LOCK))
>>> +	    write_locked_index(&the_index, &lock, COMMIT_LOCK))
>> 
>> I wondered if the next step would be to lose the "lock" parameter
>> from {hold,write}_locked_index() and have them work on a
>> process-global lock, but that would not work well.
>> 
>> The reason why this patch works is because we are only working with
>> a single destination (i.e. $GIT_INDEX_FILE typically .git/index),
>> right?
>> 
>> Interesting.
>
> Ummm, this patch wasn't supposed to be interesting. If it is then maybe
> I made a mistake...

Well, Interesting is very different from Questionable.

This lock can never have multiple active instances, as you mentioned.

We didn't have to change this for so long since the function was written; that probably is because this sequence:

	lock = hold_lock();
        use(lock);
        commit(lock); /* or rollback(lock) */

is so obviously natural to callers of the API, and commit/rollback looks very much like a declaration that we are done with the object and its resource can be freed.

This change makes sense because the API does not actually allow us to free the resource held to use this lock, so reducing the number of "struct lock_file" ever used during the life of the program makes difference, especially when you use many, even when not many of them you need to hold at the same time.

Which was counter-intuitive to me, and the realization did not hit me until I thought about it, which made it an "Interesting" change.

It may at the same be suggesting that "once in the lockfile subsystem, the resource becomes reclaimable" may be something we would want to fix, though.