Re: [GSoC Patch 0/3] Move generation, graph_pos to a slab

From: Abhishek Kumar <abhishekkumar8222@gmail.com>
To: "SZEDER Gábor" <szeder.dev@gmail.com>
Cc: stolee@gmail.com, jnareb@gmail.com, git@vger.kernel.org
Subject: Re: [GSoC Patch 0/3] Move generation, graph_pos to a slab
Date: Mon, 8 Jun 2020 11:18:27 +0530	[thread overview]
Message-ID: <20200608054827.GA2054@Abhishek-Arch> (raw)
In-Reply-To: <20200607195347.GA8232@szeder.dev>

On Sun, Jun 07, 2020 at 09:53:47PM +0200, SZEDER Gábor wrote:
> On Thu, Jun 04, 2020 at 10:22:27AM -0400, Derrick Stolee wrote:
> > On 6/4/2020 3:27 AM, Abhishek Kumar wrote:
> > > The struct commit is used in many contexts. However, members generation
> > > and graph_pos are only used for commit-graph related operations and
> > > otherwise waste memory.
> > > 
> > > This wastage would have been more pronounced as transistion to
> > > generation number v2, which uses 64-bit generation number instead of
> > > current 32-bits.
> > 
> > Thanks! This is an important step, and will already improve
> > performance in subtle ways.
> 
> While the reduced memory footprint of each commit object might improve
> performance, accessing graph position and generation numbers in a
> commit-slab is more expensive than direct field accesses in 'struct
> commit' instances.  Consequently, these patches increase the runtime
> of 'git merge-base --is-ancestor HEAD~50000 HEAD' in the linux
> repository from 0.630s to 0.940s.
> 

Thank you for checking performance. Performance penalty was something we
had discussed here [1]. 

Caching the commit slab results in local variables helped wonderfully in v2 [2].
For example, the runtime of 'git merge-base --is-ancestor HEAD~50000 HEAD'
in the linux repository increased from 0.762 to 0.767s. Since this is a
change of <1%, it is *no longer* a performance regression in my opinion.

[1]: https://lore.kernel.org/git/9a15c7ba-8b55-099a-3c59-b5e7ff6124f6@gmail.com/
[2]: https://lore.kernel.org/git/20200607193237.699335-5-abhishekkumar8222@gmail.com/

> 
> > >  create mode 100644 contrib/coccinelle/generation.cocci
> > >  create mode 100644 contrib/coccinelle/graph_pos.cocci
> > 
> > I appreciate the Coccinelle scripts to help identify
> > automatic fixes for other topics in-flight. However,
> > I wonder if they would be better placed inside the
> > existing commit.cocci file?
> 
> We add Coccinelle scripts to avoid undesirable code patterns entering
> our code base.  That, however, is not the case here: this is a
> one-time conversion, and at the end of this series 'struct commit'
> won't have a 'generation' field anymore, so once it's merged the
> compiler will catch any new 'commit->generation' accesses.  Therefore
> I don't think that these Coccinelle scripts should be added at all.
> 

Alright, that makes sense to me. Will remove in a subsequent version.

Thanks
Abhishek