From: Jeff Hostetler <git@jeffhostetler.com>
Date: Fri, 14 Apr 2017 19:32:58 GMT
Subject: Re: [PATCH v3] unpack-trees: avoid duplicate ODB lookups during checkout
Message-ID: <3746703d-54e0-a855-8511-94a573a182a8@jeffhostetler.com>
In-Reply-To: <20170414005924.sfu4csdxaoafipsv@sigill.intra.peff.net>



On 4/13/2017 8:59 PM, Jeff King wrote:
> On Thu, Apr 13, 2017 at 04:11:31PM -0700, Junio C Hamano wrote:
>
>> git@jeffhostetler.com writes:
>>
>>> +	/*
>>> +	 * Fetch the tree from the ODB for each peer directory in the
>>> +	 * n commits.
>>> +	 *
>>> +	 * For 2- and 3-way traversals, we try to avoid hitting the
>>> +	 * ODB twice for the same OID.  This should yield a nice speed
>>> +	 * up in checkouts and merges when the commits are similar.
>>> +	 *
>>> +	 * We don't bother doing the full O(n^2) search for larger n,
>>> +	 * because wider traversals don't happen that often and we
>>> +	 * avoid the search setup.
>>> +	 *
>>> +	 * When 2 peer OIDs are the same, we just copy the tree
>>> +	 * descriptor data.  This implicitly borrows the buffer
>>> +	 * data from the earlier cell.
>>
>> This "borrowing" made me worried, but it turns out that this is
>> perfectly OK.
>>
>> fill_tree_descriptor() uses read_sha1_file() to give a tree_desc its
>> own copy of the tree object data, so the code that calls into the
>> tree traversal API is responsible for freeing the buffer returned
>> from fill_tree_descriptor().  The updated code avoids double freeing
>> by setting buf[i] to NULL for borrowing [i].
>
> Yeah, I think that logic is fine. We could also keep a separate counter
> for the size of buf, like:
>
>   buf[nr_buf++] = fill_tree_descriptor(t+i, sha1);
>
> and then just count from zero to nr_buf in the freeing loop. I don't
> think it matters much either way (the free(NULL) calls are presumably
> cheap).

Agreed. I don't think this matters one way or the other, but
I'll go ahead and do this while it is fresh.
Thanks,
Jeff