Re: [PATCH 8/9] commit-graph: replace packed_oid_list with oid_array

From: Taylor Blau <me@ttaylorr.com>
Date: Dec 4, 2020, 19:14 UTC
Message-ID: <X8qKpL+mIFbjngwl@nand.local>
In-Reply-To: <X8qGqHvHbJQL9B22@coredump.intra.peff.net>

On Fri, Dec 04, 2020 at 01:57:44PM -0500, Jeff King wrote:

Show 11 quoted lines

> Our custom packed_oid_list data structure is really just an oid_array in
> disguise. Let's switch to using the generic structure, which shortens
> and simplifies the code slightly.
>
> There's one slightly awkward part: in the old code we copied a hash
> straight from the mmap'd on-disk data into the final object_id. And now
> we'll copy to a temporary oid, which we'll then pass to
> oid_array_append(). But this is an operation we have to do all over the
> commit-graph code already, since it mostly uses object_id structs
> internally. I also measured "git commit-graph --append", which triggers
> this code path, and it showed no difference.

I noticed that you also dropped the pre-allocation logic, which I think is the right thing to do (that is, removing it, not keeping it around).

It may be worth a mention here, though.

Show 8 quoted lines

> @@ -2199,26 +2177,16 @@ int write_commit_graph(struct object_directory *odb,
>  	}
>
>  	ctx->approx_nr_objects = approximate_object_count();
> -	ctx->oids.alloc = ctx->approx_nr_objects / 32;
>
> -	if (ctx->split && opts && ctx->oids.alloc > opts->max_commits)
> -		ctx->oids.alloc = opts->max_commits;

One compelling reason to drop this logic is that we only have the oid-array internals touching the .alloc variable, and we're not munging with it ourselves (running the risk of getting it out-of-sync with the actual number of bytes allocated).

> -
> -	if (ctx->append) {
> +	if (ctx->append)
>  		prepare_commit_graph_one(ctx->r, ctx->odb);

Good, this still needs to happen here.

Show 16 quoted lines

> -		if (ctx->r->objects->commit_graph)
> -			ctx->oids.alloc += ctx->r->objects->commit_graph->num_commits;
> -	}
> -
> -	if (ctx->oids.alloc < 1024)
> -		ctx->oids.alloc = 1024;
> -	ALLOC_ARRAY(ctx->oids.list, ctx->oids.alloc);
>
>  	if (ctx->append && ctx->r->objects->commit_graph) {
>  		struct commit_graph *g = ctx->r->objects->commit_graph;
>  		for (i = 0; i < g->num_commits; i++) {
> -			const unsigned char *hash = g->chunk_oid_lookup + g->hash_len * i;
> -			hashcpy(ctx->oids.list[ctx->oids.nr++].hash, hash);
> +			struct object_id oid;
> +			hashcpy(oid.hash, g->chunk_oid_lookup + g->hash_len * i);
> +			oid_array_append(&ctx->oids, &oid);

And this must be the spot that you're talking about that requires the extra copy. I think that we could certainly live with what you have here.

Thanks, Taylor