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

[Junio C Hamano <gitster@pobox.com>]

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

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