On Fri, Dec 04, 2020 at 01:57:44PM -0500, Jeff King wrote:
> 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.
> @@ -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.
> - 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
