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@xxxxxxxxx/ [2]: https://lore.kernel.org/git/20200607193237.699335-5-abhishekkumar8222@xxxxxxxxx/ > > > > 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
