v2packfile: skip decompressing and hashing blobs in add_promisor_object()

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.

This improves performance for very large pack files significantly:

 $ time ~/git/git/git-rev-list --objects --all --exclude-promisor-objects --quiet
 ________________________________________________________
 Executed in  117.63 secs    fish           external
    usr time   45.56 secs    1.09 millis   45.56 secs
    sys time   37.91 secs    1.05 millis   37.91 secs

Signed-off-by: Aaron Plattner <aplattner@nvidia.com>
---
v2: Fix PARSE_OBJECT_SKIP_HASH_CHECK with UNINTERESTING objects, use it
in parse_object_with_flags.

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

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

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.

The patch itself looks great to me.

-Peff

On 12/5/25 6:06 PM, Jeff King wrote:

I was rewriting the commit message for that part to justify why it's safe to use PARSE_OBJECT_SKIP_HASH_CHECK, and now I'm questioning it. :)

It definitely seems fine for blobs but if what we're trying to check for is on-disk corruption, maybe it's not a good idea to skip it for other objects since we're actually using their contents here.

I still think the OBJ_NONE fix is worthwhile and I'll send that out separately, but maybe it would be a good idea to split PARSE_OBJECT_SKIP_HASH_CHECK into separate flags for blobs vs. all objects? Or just go back to v1 of the add_promisor_object() patch? Or do you think this version is okay despite that concern?

-- Aaron

On Sat, Dec 06, 2025 at 11:40:01AM -0800, Aaron Plattner wrote:

I think it's OK to skip the hashes here. In some code paths we really care about checking the consistency of the objects (like "rev-list --verify", or fsck). But if the caller knows that we are not trying to do that, then that opens the doors to other optimizations like avoiding object loads entirely.

You could argue that if we _do_ load an object, we might as well hash it to check its consistency. IMHO there is not much value in that, as the cost is not totally trivial, it's not a thorough validation of what's on disk (because we are skipping some objects), and this sort of bit-flipping corruption is pretty rare in the first place.

So to my mind, there are really two types of callers that want to parse an object: ones that are verifying database consistency and want to be thorough, and ones that want things to be as fast as possible. And I think the caller here (traversing the promisor objects) is in the latter camp. E.g., if we ever added a secondary index of "these are all the promised objects we don't have" we would just use that!

So really, I think all I was suggesting for the commit message is to say "this is the kind of caller that wants things to be as fast as possible". ;)

The ideal series to me is probably:

  - patch 1 fixes the OBJ_NONE issues. It would be great if there was
    some way to show the impact of this bug on an existing case, but I
    imagine it would be quite hard. We'd never produce the wrong answer
    but just do things slowly. So you'd need a case where we end up with
    OBJ_NONE, and then a SKIP_HASH code path that loads a big blob. It's
    easy to find the latter (just hand rev-list a blob on the command
    line), but the former is harder. I doubt it's worth the effort to
    dig for one, since we know your case in patch 2 will demonstrate it.

  - patch 2 uses SKIP_HASH

-Peff