Re: [PATCH v2] packfile: skip decompressing and hashing blobs in add_promisor_object()

From: Jeff King <peff@peff.net>
Date: Dec 6, 2025, 02:06 UTC
Message-ID: <20251206020648.GB1714099@coredump.intra.peff.net>
In-Reply-To: <20251206002014.2066644-1-aplattner@nvidia.com>

On Fri, Dec 05, 2025 at 04:20:12PM -0800, Aaron Plattner wrote:

Show 25 quoted lines

> When is_promisor_object() is called for the first time, it lazily
> initializes a set of all promisor objects by iterating through all
> objects in promisor packs. For each object, add_promisor_object() calls
> parse_object(), which decompresses and hashes the entire object.
> 
> For repositories with large pack files, this can take an extremely long
> time. For example, on a production repository with a 176 GB promisor
> pack:
> 
>  $ time ~/git/git/git-rev-list --objects --all --exclude-promisor-objects --quiet
>  ________________________________________________________
>  Executed in   76.10 mins    fish           external
>     usr time   72.10 mins    1.83 millis   72.10 mins
>     sys time    3.56 mins    0.17 millis    3.56 mins
> 
> add_promisor_object() needs the full object for trees, commits, and
> tags. But blobs contain no references to other objects, so the function
> can just insert their oids into the set and move on.
> 
> parse_object_with_flags() has code to skip decompressing blobs, but it
> unfortunately doesn't work with the objects created by
> mark_uninteresting() because they have obj->type == OBJ_NONE. Update
> parse_object_with_flags() to handle blobs and trees that are in this
> state, and then update add_promisor_object() to use
> PARSE_OBJECT_SKIP_HASH_CHECK.

Good catch on the matching tree code. It doesn't trigger for your use case (the caller has to pass in the DISCARD_TREE flag), but it's a lurking bug nonetheless.

I'm tempted to say that those changes in parse_object_with_flags() should happen as a separate patch, since they really are fixing an existing bug. But I can live with it all as one, too.

One other thing it might be worth thinking about or mentioning in the commit message: we are skipping the hash check on all objects now (not just blobs). I think this is OK to do along the lines of discussion in c868d8e91f (parse_object(): allow skipping hash check, 2022-09-06). I dunno. Maybe it is kind of self-evident that not every operation needs to do a consistency check of every object.

>  object.c   | 4 ++--
>  packfile.c | 3 ++-
>  2 files changed, 4 insertions(+), 3 deletions(-)

The patch itself looks great to me.

-Peff