[0/1] commit-graph: avoid unnecessary tag deference when merging

Message

Taylor Blau March 21, 2020, 3:44 a.m. UTC

Hi,

Here's a patch that Peff and I were kicking around from a couple of
months ago when we were investigating deploying commit-graphs at GitHub.
Particularly, we were looking into why merging two commit-graphs after a
write with '--split' took as much time as we were seeing in our
infrastructure.

This patch avoids an unnecessary tag dereference in
'merge_commit_graph()', which can improve the running time of a
commit-graph write by around ~7.4% on average.

Thanks in advance for your review.

Taylor Blau (1):
  commit-graph.c: avoid unnecessary tag dereference when merging

 commit-graph.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

--
2.26.0.rc2.311.g8e52d2684b

Comments

Junio C Hamano March 21, 2020, 4:56 a.m. UTC | #1

Taylor Blau <[e-mail]> writes:

> This patch avoids an unnecessary tag dereference in
> 'merge_commit_graph()', which can improve the running time of a
> commit-graph write by around ~7.4% on average.

That I suspect depends heavily on what portion of your total
committishes consist of tags, no (in an absurdly extreme case, if
there is no tag in the repository, there won't be any improvement)?

What tag-vs-commit ratio is the above "average" number based on?

Thanks.

Jeff King March 21, 2020, 5:04 a.m. UTC | #2

On Fri, Mar 20, 2020 at 09:56:16PM -0700, Junio C Hamano wrote:

> Taylor Blau <[e-mail]> writes:
> 
> > This patch avoids an unnecessary tag dereference in
> > 'merge_commit_graph()', which can improve the running time of a
> > commit-graph write by around ~7.4% on average.
> 
> That I suspect depends heavily on what portion of your total
> committishes consist of tags, no (in an absurdly extreme case, if
> there is no tag in the repository, there won't be any improvement)?
> 
> What tag-vs-commit ratio is the above "average" number based on?

I think the point is that in this code path we've already identified the
candidates as commits (because they were in an existing commit-graph
file), so treating the items as a committishes in the first place is
unnecessary. If an object isn't itself a commit, we should be dropping
it from the proposed output (possibly we ought to give a warning in such
a case, too, as it indicates the previous writer did something wrong).

-Peff

Taylor Blau March 21, 2020, 6:12 a.m. UTC | #3

On Sat, Mar 21, 2020 at 01:04:55AM -0400, Jeff King wrote:
> On Fri, Mar 20, 2020 at 09:56:16PM -0700, Junio C Hamano wrote:
>
> > Taylor Blau <[e-mail]> writes:
> >
> > > This patch avoids an unnecessary tag dereference in
> > > 'merge_commit_graph()', which can improve the running time of a
> > > commit-graph write by around ~7.4% on average.
> >
> > That I suspect depends heavily on what portion of your total
> > committishes consist of tags, no (in an absurdly extreme case, if
> > there is no tag in the repository, there won't be any improvement)?
> >
> > What tag-vs-commit ratio is the above "average" number based on?
>
> I think the point is that in this code path we've already identified the
> candidates as commits (because they were in an existing commit-graph
> file), so treating the items as a committishes in the first place is
> unnecessary. If an object isn't itself a commit, we should be dropping
> it from the proposed output (possibly we ought to give a warning in such
> a case, too, as it indicates the previous writer did something wrong).

Yes, exactly. Thank you.

> -Peff

Thanks,
Taylor