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