On Mon, Jun 08, 2020 at 09:45:12AM -0400, Derrick Stolee wrote: > On 6/8/2020 4:36 AM, SZEDER Gábor wrote: > > On Mon, Jun 08, 2020 at 11:18:27AM +0530, Abhishek Kumar wrote: > >> 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. > > > > Interesting, I measured 0.870s with v2, still a notable increase from > > 0.630s. > > This is an interesting point. The --is-ancestor is critical to the > performance issue (as measured on my machine). > > For "git merge-base HEAD~50000 HEAD" on the Linux repo, I get > > v2.27.0: > real 0m0.515s > user 0m0.467s > sys 0m0.048s > > v2 series: > real 0m0.534s > user 0m0.481s > sys 0m0.053s I, too, see similarly small differences in this case. > With "--is-ancestor" I see the following: > > v2.27.0: > real 0m0.591s > user 0m0.539s > sys 0m0.052s > > v2 series: > real 0m0.773s > user 0m0.733s > sys 0m0.040s > > The --is-ancestor option [1] says > > Check if the first <commit> is an ancestor of the second > <commit>, and exit with status 0 if true, or with status > 1 if not. Errors are signaled by a non-zero status that > is not 1. > > [1] https://git-scm.com/docs/git-merge-base#Documentation/git-merge-base.txt---is-ancestor > > This _should_ be faster than "git branch --contains HEAD~50000", > but it is much much slower: > > $ time git branch --contains HEAD~50000 > real 0m0.068s > user 0m0.061s > sys 0m0.008s > > So, there is definitely something going on that slows the > "--is-ancestor" path in this case. But, the solution is not > to halt the current patch (which likely has memory footprint > benefits when dealing with a lot of tree and blob objects) > and instead fix the underlying algorithm. Other, more common cases are affected as well, notably the simple 'git rev-list --topo-order': performance: 1.226479734 s: git command: /home/szeder/src/git/BUILDS/v2.27.0/bin/git rev-list --topo-order HEAD max RSS: 162400k performance: 1.741309536 s: git command: /home/szeder/src/git/git rev-list --topo-order HEAD max RSS: 169556k Is the supposed memory footprint reduction that large to justify this runtime increase? > Let's add that to the list of things to do. And to the commit messages. > >>> 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. > > I disagree. We _also_ add Coccinelle scripts when doing one-time > refactors to avoid logical merge conflicts with other topics in > flight. If someone else is working on a parallel topic that adds > references to graph_pos or generation member, then the scripts provide > an easy way for the maintainer to update those references in the merge > commit. Alternatively, the contributor could rebase on top of this > series and run the scripts themselves to fix their patches before > submission. > > For example, this was done carefully in the sha->object_id > conversion using contrib/coccinelle/object_id.cocci. 'object_id.cocci' is not about sha->object_id conversions, but about avoiding undesirable code patterns, e.g. we prefer oideq() over !oidcmp(), and the compiler, of course, can't help to catch that. Coccinelle scripts used for actual sha->object_id transformations were not added to 'object_id.cocci', but were recorded only in the commit messages for reference, see e.g. 9b56149996 (merge-recursive: convert struct merge_file_info to object_id, 2016-06-24) and a couple of its ancestors.
