From: Aaron Plattner <aplattner@nvidia.com>
Date: Fri, 05 Dec 2025 18:50:02 GMT
Subject: Re: [PATCH] packfile: skip decompressing and hashing blobs in add_promisor_object()
Message-ID: <235d80bd-2516-47f9-958f-0e5a16892758@nvidia.com>
In-Reply-To: <20251205180106.GC18566@coredump.intra.peff.net>

On 12/5/25 10:01 AM, Jeff King wrote:
> On Fri, Dec 05, 2025 at 12:48:54PM -0500, Jeff King wrote:
> 
>> OK, so we are checking the type up front and then skipping
>> parse_object() if we can. But there is already some logic inside
>> parse_object() for these kinds of optimizations. If we tell it we are
>> not interested in checking the hash of the objects, then it knows it can
>> skip loading the blob entirely.
>>
>> But it can _also_ use that flag for other things, like using the
>> commit-graph rather than loading individual commit objects. So doing
>> this:
>>    
>> diff --git a/packfile.c b/packfile.c
>> index 9cc11b6dc5..01b992a4e1 100644
>> --- a/packfile.c
>> +++ b/packfile.c
>> @@ -2310,7 +2310,8 @@ static int add_promisor_object(const struct object_id *oid,
>>   		we_parsed_object = 0;
>>   	} else {
>>   		we_parsed_object = 1;
>> -		obj = parse_object(pack->repo, oid);
>> +		obj = parse_object_with_flags(pack->repo, oid,
>> +					      PARSE_OBJECT_SKIP_HASH_CHECK);
>>   	}
>>   
>>   	if (!obj)
>>
>> drops my linux.git case down to 49s. It's skipping the blobs (with no
>> need for your patch) and loading the commits out of the graph file. Note
>> that you may need to "git commit-graph write --reachable" to see the
>> effect (I think we do generate graphs by default in git-gc these days,
>> but I'm not sure if we do so right after cloning).
> 
> Oh, and obviously it is skipping the hash computation on the objects,
> too. That's probably not as important as avoiding the object loads in
> the first place, but it may also be making a measurable difference on
> the ones we do load (notably trees here).

Thanks! I had looked at PARSE_OBJECT_SKIP_HASH_CHECK but it wasn't 
obvious to me whether it could be used here or not.

Unfortunately, setting that flag doesn't seem to improve performance for 
me because in parse_object_with_flags(), lookup_object() returns an obj 
pointer with obj->parsed == 0 and obj->type == OBJ_NONE. So it skips 
this block and ends up inflating the object anyway:

	if ((!obj || obj->type == OBJ_BLOB) &&
	    odb_read_object_info(r->objects, oid, NULL) == OBJ_BLOB) {
		if (!skip_hash && stream_object_signature(r, repl) < 0) {
			error(_("hash mismatch %s"), oid_to_hex(oid));
			return NULL;
		}
		parse_blob_buffer(lookup_blob(r, oid));
		return lookup_object(r, oid);
	}

I was confused about why the check was structured that way, but reading 
the description of commit 8db2dad7a045e376b9c4f51ddd33da43c962e3a4 
cleared that up. Thank you for thoroughly documenting that!

Are OBJ_NONE objects expected here? Should the check be

	if ((!obj || obj->type == OBJ_NONE || obj->type == OBJ_BLOB) &&
	    odb_read_object_info(r->objects, oid, NULL) == OBJ_BLOB) {
	    [...]
	}

?

If I make that change combined with your PARSE_OBJECT_SKIP_HASH_CHECK 
change then the time drops to 1:58, so that's great!

> -Peff