From: Junio C Hamano Date: Sat, 21 Mar 2020 05:01:54 GMT Subject: Re: [PATCH 1/1] commit-graph.c: avoid unnecessary tag dereference when merging Message-ID: In-Reply-To: <4c79a9ea909ebff8c0987bcf95692da92e79bda4.1584762087.git.me@ttaylorr.com> Taylor Blau writes: > When performing a 'git commit-graph write' with '--split', the > commit-graph machinery calls 'merge_commit_graph()' after deciding on a > split strategy to optionally clean up any existing commit-graph > layers that were made obsolete by the split strategy [1]. > > At this time, 'merge_commit_graph()' checks each commit that it writes > into the merged graph to make sure that it still exists in the object > store. > > To do this, it uses 'lookup_commit_reference_gently()', which accepts > either a commit object, or a tag that refers to a commit. However, since > all 'oid' arguments passed to this function are from within the > commit-graphs being merged, we never pass a commit reference, and so any > time we spend in 'deref_tag()' is wasted. Ahh, so my question on the cover letter was utterly off the mark. It is that feeding a commit to deref_tag() is unneeded. It is quite surprising to hear that deref_tag() call is _so_ expensive that it wastes 7% of the total cycles. The patch itself looks good. Thanks. > for (i = 0; i < g->num_commits; i++) { > struct object_id oid; > - struct commit *result; > + struct commit *result = NULL; > > display_progress(ctx->progress, i + 1); > > load_oid_from_graph(g, i + offset, &oid); > > /* only add commits if they still exist in the repo */ > - result = lookup_commit_reference_gently(ctx->r, &oid, 1); > + if (repo_has_object_file(ctx->r, &oid)) { > + result = lookup_commit(ctx->r, &oid); > + if (repo_parse_commit(ctx->r, result)) > + result = NULL; > + } > > if (result) { > ctx->commits.list[ctx->commits.nr] = result;