Re: [PATCH 2/2] pack-objects: prefetch objects to be packed

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jul 21, 2020, 16:37 UTC
Message-ID: <20200721163736.69610-1-jonathantanmy@google.com>
In-Reply-To: <xmqqd04p8ywt.fsf@gitster.c.googlers.com>

Show 29 quoted lines

> Hmph, the resulting codeflow structure feels somewhat iffy.  Perhaps
> I am not reading the code correctly, but
> 
>  * There is a loop that scans from 0..to_pack.nr_objects and calls
>    check_object() for each and every one of them;
> 
>  * The called check_object(), when it notices that a missing and
>    promised (i.e. to be lazily fetched) object is in the to_pack
>    array, asks prefetch_to_pack() to scan from that point to the end
>    of that array and grabs all of them that are missing.
> 
> It almost feels a lot cleaner to see what is going on in the
> resulting code, instead of the way the new "loop" was added, if a
> new loop is added _before_ the loop to call check_object() on all
> objects in to_pack array as a pre-processing phase when there is a
> promisor remote.  That is, after reverting all the change this patch
> makes to check_object(), add a new loop in get_object_details() that
> looks more or less like so:
> 
> 	QSORT(sorted_by_offset, to_pack.nr_objects, pack_offset_sort);
> 
> +	if (has_promisor_remote())
> +		prefetch_to_pack(0);
> +
> 	for (i = 0; i < to_pack.nr_objects; i++) {
> 
> 
> Was the patch done this way because scanning the entire array twice
> is expensive?

Yes. If we called prefetch_to_pack(0) first (as you suggest), this first scan involves checking the existence of all objects in the array, so I thought it would be expensive. (Checking the existence of an object probably brings the corresponding pack index into disk cache on platforms like Linux, so 2 object reads might not take much more time than 1 object read, but I didn't want to rely on this when I could just avoid the extra read.)

Show 5 quoted lines

> The optimization makes sense to me if certain
> conditions are met, like...
> 
>  - Most of the time there is no missing object due to promisor, even
>    if has_promissor_to_remote() is true;

I think that optimizing for this condition makes sense - most pushes (I would think) are pushes of objects we create locally, and thus no objects are missing.

Show 5 quoted lines

>  - When there are missing objects due to promisor, pack_offset_sort
>    will keep them near the end of the array; and
> 
>  - Given the oid, oid_object_info_extended() on it with
>    OBJECT_INFO_FOR_PREFETCH is expensive.

I see this as expensive since it involves checking of object existence.

> Only when all these conditions are met, it would avoid unnecessary
> overhead by scanning only a very later part of the array by delaying
> the point in the array where prefetch_to_pack() starts scanning.

Yes (and when there are no missing objects at all, there is no double-scanning).