Re: [PATCH v2 2/2] diff: restrict when prefetching occurs

From: Junio C Hamano <gitster@pobox.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org, stolee@gmail.com, peff@peff.net
Subject: Re: [PATCH v2 2/2] diff: restrict when prefetching occurs
Date: Thu, 02 Apr 2020 16:25:16 -0700	[thread overview]
Message-ID: <xmqqy2rd1ndv.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200402230937.47323-1-jonathantanmy@google.com> (Jonathan Tan's message of "Thu, 2 Apr 2020 16:09:37 -0700")

Jonathan Tan <jonathantanmy@google.com> writes:

>> This comment makes me wonder if it would be even better to
>> 
>>  - prepare an empty to_fetch OID array in the caller,
>> 
>>  - if the output format is one of the ones that wants prefetch, add
>>    object names to to_fetch in the caller, BUT not fetch there.
>> 
>>  - pass &to_fetch by the caller to this function, and this code here
>>    may add even more objects,
>> 
>>  - then do the prefetch here (so a single promisor interaction will
>>    grab objects the caller would have fetched before calling us and
>>    the ones we want here), and then clear the to_fetch array.
>> 
>>  - the caller, after seeing this function returns, checks to_fetch
>>    and if it is not empty, fetches (i.e. the caller prepared list of
>>    objects based on the output type, we ended up not calling this
>>    helper, and then finally the caller does the prefetch).
>> 
>> That way, the "unless we have already prefetched" logic can go, and
>> we can lose one indentation level, no?
>
> This means that the only prefetch occurs in diffcore_rename()?

No, but I phrased the last bullet item incorrectly.  "after seeing
this function returns" is wrong, but what is in parentheses (i.e. if
we didn't call diffcore_rename) is correct.

> I don't
> think this will work for 2 reasons:
>
>  - diffcore_std() calls diffcore_break() (which also reads blobs) before
>    diffcore_rename()

Ahh, I missed that part.

>  - (more importantly) there's a code path in diffcore_std() that does
>    not call diffcore_rename(), so we would still need some prefetching
>    logic in diffcore_std() in case diffcore_rename() is not called

That one I think is already covered.