Re: [PATCH] cache-tree: avoid needless promisor object fetch

From: Jonathan Tan <jonathantanmy@google.com>
Date: Apr 26, 2021, 19:49 UTC
Message-ID: <20210426194928.326338-1-jonathantanmy@google.com>
In-Reply-To: <nycvar.QRO.7.76.6.2104231409500.54@tvgsbejvaqbjf.bet>

Show 17 quoted lines

> > In update_one() (used only by cache_tree_update()), there is an object
> > existence check that, if it fails, will automatically trigger a lazy
> > fetch in a partial clone. But the fetch is not necessary - the object is
> > not actually being used.
> 
> I find it curious, though, that the `ce_missing_ok` variable is defined
> thusly (sadly, the context of your diff is too small to show it):
> 
>                 ce_missing_ok = mode == S_IFGITLINK || missing_ok ||
>                         (has_promisor_remote() &&
>                          ce_skip_worktree(ce));
> 
> Which means that the `has_object_file()` function is only called if the
> entry is not marked with the `skip-worktree` bit, i.e. if it is _not_
> excluded from the sparse checkout.
> 
> Wouldn't that mean that the object _should_ be there?

In a partial clone, probably not?

> I guess what I am saying is that while the commit message focuses on the
> "What?" of the patch, I would love to hear more about the "Why?". And
> maybe the "When?" as in: when does this actually matter?

In this case, that's something I'd like help in figuring out too. Normally this code path (unpack_trees()) prefetches everything through a call to check_updates(), but the update flag is somehow not set so there is no prefetching happening.

> And since the bug was critical enough for you to spend time on crafting
> it, maybe it would make sense to add a regression test to ensure that this
> bug does not creep in again?

OK.

Show 6 quoted lines

> > Replace that check with two checks: an object existence check that does
> > not fetch, and then a check that that object is a promisor object.
> 
> This essentially repeats what the diff says, but it might make more sense
> to explain why the post-image of this diff is more correct (and maybe
> discuss performance implications).

OK - I think this is the "why" and "when" you described above.

Show 6 quoted lines

> > Doing this avoids multiple lazy fetches when merging two trees in a
> > partial clone, as noticed at $DAYJOB.
> 
> Ah. But where are those trees fetched, then?
> 
> Maybe lead with the description of the bug?

This was a partial clone excluding blobs only. I'll update the commit message to mention this detail.

Show 10 quoted lines

> > Another alternative is to think about whether the object existence check
> > here is needed in the first place.
> >
> > There might also be other places we can make a similar change in
> > update_one(), but I limited myself to what's needed to solve the
> > specific case we discovered at $DAYJOB.
> 
> I only see another `has_object_file()` call site at the very beginning,
> and I think this needs to fetch. Or maybe it is more efficient to
> construct the cache tree from scratch than fetch it?

Good point - if we can construct it, we probably shouldn't fetch it.

> There is also `cache_tree_fully_valid_1()`, where I think the same
> handling could potentially make sense. (Or, if you target `seen`,
> `cache_tree_fully_valid()`.

True.