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

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:

quoted

On 6/4/2020 3:27 AM, Abhishek Kumar wrote:

quoted

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/ (local)
[2]: https://lore.kernel.org/git/20200607193237.699335-5-abhishekkumar8222@gmail.com/ (local)

quoted

quoted

 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