{"thread":{"id":"50375","subject":"Comparing rebase --am with --interactive via p3400","startedAt":"2019-02-01T06:05:26Z","lastAt":"2020-04-04T20:33:55Z","messageCount":11,"participants":["Johannes Schindelin","Elijah Newren","Alban Gruin","Phillip Wood"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"368298","messageId":"nycvar.QRO.7.76.6.1901312310280.41@tvgsbejvaqbjf.bet","threadId":"50375","inReplyTo":null,"subject":"Comparing rebase --am with --interactive via p3400","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-01T06:04:31Z","receivedAt":"2019-02-01T06:05:26Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nas discussed at the Contributors' Summit, I ran p3400 as-is (i.e. with the\n--am backend) and then with --keep-empty to force the interactive backend\nto be used. Here are the best of 10, on my relatively powerful Windows 10\nlaptop, with current `master`.\n\nWith regular rebase --am:\n\n3400.2: rebase on top of a lot of unrelated changes             5.32(0.06+0.15)\n3400.4: rebase a lot of unrelated changes without split-index   33.08(0.04+0.18)\n3400.6: rebase a lot of unrelated changes with split-index      30.29(0.03+0.18)\n\nwith --keep-empty to force the interactive backend:\n\n3400.2: rebase on top of a lot of unrelated changes             3.92(0.03+0.18)\n3400.4: rebase a lot of unrelated changes without split-index   33.92(0.03+0.22)\n3400.6: rebase a lot of unrelated changes with split-index      38.82(0.03+0.16)\n\nI then changed it to -m to test the current scripted version, trying to\nlet it run overnight, but my laptop eventually went to sleep and the tests\nwere not even done. I'll let them continue and report back.\n\nMy conclusion after seeing these numbers is: the interactive rebase is\nreally close to the performance of the --am backend. So to me, it makes a\ntotal lot of sense to switch --merge over to it, and to make --merge the\ndefault. We still should investigate why the split-index performance is so\nsignificantly worse, though.\n\nCiao,\nDscho\n"},{"id":"368303","messageId":"nycvar.QRO.7.76.6.1902010819470.41@tvgsbejvaqbjf.bet","threadId":"50375","inReplyTo":"nycvar.QRO.7.76.6.1901312310280.41@tvgsbejvaqbjf.bet","subject":"Re: Comparing rebase --am with --interactive via p3400","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-01T07:22:20Z","receivedAt":"2019-02-01T07:22:24Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Fri, 1 Feb 2019, Johannes Schindelin wrote:\n\n> as discussed at the Contributors' Summit, I ran p3400 as-is (i.e. with the\n> --am backend) and then with --keep-empty to force the interactive backend\n> to be used. Here are the best of 10, on my relatively powerful Windows 10\n> laptop, with current `master`.\n> \n> With regular rebase --am:\n> \n> 3400.2: rebase on top of a lot of unrelated changes             5.32(0.06+0.15)\n> 3400.4: rebase a lot of unrelated changes without split-index   33.08(0.04+0.18)\n> 3400.6: rebase a lot of unrelated changes with split-index      30.29(0.03+0.18)\n> \n> with --keep-empty to force the interactive backend:\n> \n> 3400.2: rebase on top of a lot of unrelated changes             3.92(0.03+0.18)\n> 3400.4: rebase a lot of unrelated changes without split-index   33.92(0.03+0.22)\n> 3400.6: rebase a lot of unrelated changes with split-index      38.82(0.03+0.16)\n> \n> I then changed it to -m to test the current scripted version, trying to\n> let it run overnight, but my laptop eventually went to sleep and the tests\n> were not even done. I'll let them continue and report back.\n\nIt finally finished:\n\n3400.2: rebase on top of a lot of unrelated changes             7.37(0.09+0.19) \n3400.4: rebase a lot of unrelated changes without split-index 393.96(0.04+0.15)\n3400.6: rebase a lot of unrelated changes with split-index    404.65(0.01+0.24)\n\nSo there is a seemingly significant cost to using the split-index that is\njust very unfortunate. In any case, just switching from the scripted\n--merge backend to the built-in interactive backend results in a >10x\nfaster execution. So I *definitely* want that scripted `--merge` backend\nto go away. Thank you for doing this.\n\nCiao,\nDscho\n"},{"id":"368317","messageId":"CABPp-BF=ev03WgODk6TMQmuNoatg2kiEe5DR__gJ0OTVqHSnfQ@mail.gmail.com","threadId":"50375","inReplyTo":"nycvar.QRO.7.76.6.1901312310280.41@tvgsbejvaqbjf.bet","subject":"Re: Comparing rebase --am with --interactive via p3400","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-02-01T09:26:53Z","receivedAt":"2019-02-01T09:27:08Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Dscho,\n\nOn Thu, Jan 31, 2019 at 10:04 PM Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n>\n> Hi Elijah,\n>\n> as discussed at the Contributors' Summit, I ran p3400 as-is (i.e. with the\n> --am backend) and then with --keep-empty to force the interactive backend\n> to be used. Here are the best of 10, on my relatively powerful Windows 10\n> laptop, with current `master`.\n>\n> With regular rebase --am:\n>\n> 3400.2: rebase on top of a lot of unrelated changes             5.32(0.06+0.15)\n> 3400.4: rebase a lot of unrelated changes without split-index   33.08(0.04+0.18)\n> 3400.6: rebase a lot of unrelated changes with split-index      30.29(0.03+0.18)\n>\n> with --keep-empty to force the interactive backend:\n>\n> 3400.2: rebase on top of a lot of unrelated changes             3.92(0.03+0.18)\n> 3400.4: rebase a lot of unrelated changes without split-index   33.92(0.03+0.22)\n> 3400.6: rebase a lot of unrelated changes with split-index      38.82(0.03+0.16)\n\nAwesome, thanks for checking that out.  I ran on both linux and mac\nand saw similar relative performances.  Comparing am-based rebase to\nan implied-interactive rebase on both linux and mac (with a version of\ngit including en/rebase-merge-on-sequencer so that -m gives the same\nperformance that you'd see with --keep-empty), I saw:\n\nOn Linux:\n\nam-based rebase (without -m):\n\n3400.2: rebase on top of a lot of unrelated changes             1.87(1.64+0.21)\n3400.4: rebase a lot of unrelated changes without split-index   7.87(6.24+1.00)\n3400.6: rebase a lot of unrelated changes with split-index      5.99(5.05+0.67)\n\ninteractive-machinery rebase (with -m):\n\n3400.2: rebase on top of a lot of unrelated changes             1.80(1.60+0.19)\n3400.4: rebase a lot of unrelated changes without split-index   6.78(5.70+0.91)\n3400.6: rebase a lot of unrelated changes with split-index      6.92(5.70+0.89)\n\n\nOn Mac:\n\nam-based rebase (without -m):\n\nTest                                                            this tree\n-------------------------------------------------------------------------------\n3400.2: rebase on top of a lot of unrelated changes             2.68(1.68+0.68)\n3400.4: rebase a lot of unrelated changes without split-index   8.89(5.86+2.94)\n3400.6: rebase a lot of unrelated changes with split-index      7.87(5.35+2.51)\n\n\ninteractive-machinery rebase (with -m):\n\nTest                                                            this tree\n-------------------------------------------------------------------------------\n3400.2: rebase on top of a lot of unrelated changes             1.99(1.61+0.77)\n3400.4: rebase a lot of unrelated changes without split-index   8.63(5.38+3.38)\n3400.6: rebase a lot of unrelated changes with split-index      9.36(5.53+3.95)\n\n> I then changed it to -m to test the current scripted version, trying to\n> let it run overnight, but my laptop eventually went to sleep and the tests\n> were not even done. I'll let them continue and report back.\n>\n> My conclusion after seeing these numbers is: the interactive rebase is\n> really close to the performance of the --am backend. So to me, it makes a\n> total lot of sense to switch --merge over to it, and to make --merge the\n> default. We still should investigate why the split-index performance is so\n> significantly worse, though.\n\nCool, I'll update my patches to make --merge the default (building on\ntop of en/rebase-merge-on-sequencer) and post it as an RFC.  But yeah,\nwe should also check into why the split-index performance becomes a\nbit worse with such a change.\n\nThanks,\nElijah\n"},{"id":"389004","messageId":"a00e1689-ec7c-4039-a2e9-f72d452ae4ff@gmail.com","threadId":"50375","inReplyTo":"nycvar.QRO.7.76.6.1901312310280.41@tvgsbejvaqbjf.bet","subject":"Re: Comparing rebase --am with --interactive via p3400","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-12-27T21:11:36Z","receivedAt":"2019-12-27T21:15:38Z","isPatch":false,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Johannes & Elijah,\n\nLe 01/02/2019 à 07:04, Johannes Schindelin a écrit :\n> Hi Elijah,\n> \n> as discussed at the Contributors' Summit, I ran p3400 as-is (i.e. with the\n> --am backend) and then with --keep-empty to force the interactive backend\n> to be used. Here are the best of 10, on my relatively powerful Windows 10\n> laptop, with current `master`.\n> \n> With regular rebase --am:\n> \n> 3400.2: rebase on top of a lot of unrelated changes             5.32(0.06+0.15)\n> 3400.4: rebase a lot of unrelated changes without split-index   33.08(0.04+0.18)\n> 3400.6: rebase a lot of unrelated changes with split-index      30.29(0.03+0.18)\n> \n> with --keep-empty to force the interactive backend:\n> \n> 3400.2: rebase on top of a lot of unrelated changes             3.92(0.03+0.18)\n> 3400.4: rebase a lot of unrelated changes without split-index   33.92(0.03+0.22)\n> 3400.6: rebase a lot of unrelated changes with split-index      38.82(0.03+0.16)\n> \n> I then changed it to -m to test the current scripted version, trying to\n> let it run overnight, but my laptop eventually went to sleep and the tests\n> were not even done. I'll let them continue and report back.\n> \n> My conclusion after seeing these numbers is: the interactive rebase is\n> really close to the performance of the --am backend. So to me, it makes a\n> total lot of sense to switch --merge over to it, and to make --merge the\n> default. We still should investigate why the split-index performance is so\n> significantly worse, though.\n> \n> Ciao,\n> Dscho\n> \n\nI investigated a bit on this.  From a quick glance at a callgrind trace,\nI can see that ce_write_entry() is called 20 601[1] times with `git am',\nbut 739 802 times with the sequencer when the split-index is enabled.\n\nFor reference, here are the timings, measured on my Linux machine, on a\ntmpfs, with git.git as the repo:\n\n`rebase --am':\n> 3400.2: rebase on top of a lot of unrelated changes             0.29(0.24+0.03)\n> 3400.4: rebase a lot of unrelated changes without split-index   6.77(6.51+0.22)\n> 3400.6: rebase a lot of unrelated changes with split-index      4.43(4.29+0.13)\n`rebase --quiet':\n> 3400.2: rebase on top of a lot of unrelated changes             0.24(0.21+0.02)\n> 3400.4: rebase a lot of unrelated changes without split-index   5.60(5.32+0.27)\n> 3400.6: rebase a lot of unrelated changes with split-index      5.67(5.40+0.26)\n\nThis comes from two things:\n\n1. There is not enough shared entries in the index with the sequencer.\n\ndo_write_index() is called only by do_write_locked_index() with `--am',\nbut is also called by write_shared_index() with the sequencer once for\nevery other commit.  As the latter is only called by\nwrite_locked_index(), which means that too_many_not_shared_entries()\nreturns true for the sequencer, but never for `--am'.\n\nRemoving the call to discard_index() in do_pick_commit() (as in the\nfirst attached patch) solve this particular issue, but this would\nrequire a more thorough analysis to see if it is actually safe to do.\n\nAfter this, ce_write() is still called much more by the sequencer.\n\nHere are the results of `rebase --quiet' without discarding the index:\n\n> 3400.2: rebase on top of a lot of unrelated changes             0.23(0.19+0.04)\n> 3400.4: rebase a lot of unrelated changes without split-index   5.14(4.95+0.18)\n> 3400.6: rebase a lot of unrelated changes with split-index      5.02(4.87+0.15)\nThe performance of the rebase is better in the two cases.\n\n\n2. The base index is dropped by unpack_trees_start() and unpack_trees().\n\nNow, write_shared_index() is no longer called and write_locked_index()\nis less expensive than before according to callgrind.  But\nce_write_entry() is still called 749 302 times (which is even more than\nbefore.)\n\nThe only place where ce_write_entry() is called is in a loop in\ndo_write_index().  The number of iterations is dictated by the size of\nthe cache, and there is a trace2 probe dumping this value.\n\nFor `--am', the value goes like this: 1, 2, 2, 2, 2, 3, 3, 3, 3, 4, 4,\n4, 4, 5, 5, 5, 5, … up until 101.\n\nFor the sequencer, it goes like this: 1, 1, 3697, 3697, 3698, 3698,\n3699, 3699, … up until 3796.\n\nThe size of the cache is set in prepare_to_write_split_index().  It\ngrows if a cache entry has no index (most of them should have one by\nnow), or if the split index has no base index (with `--am', the split\nindex always has a base.)  This comes from unpack_trees_start() -- it\ncreates a new index, and unpack_trees() does not carry the base index,\nhence the size of the cache.\n\nThe second attached patch (which is broken for the non-interactive\nrebase case) demonstrates what we could expect for the split-index case\nif we fix this:\n\n> 3400.2: rebase on top of a lot of unrelated changes             0.24(0.21+0.03)\n> 3400.4: rebase a lot of unrelated changes without split-index   5.81(5.62+0.17)\n> 3400.6: rebase a lot of unrelated changes with split-index      4.76(4.54+0.20)\nSo, for everything related to the index, I think that’s it.\n\n[1] Numbers may vary, but they should remain in the same order of magnitude.\n\nCheers,\nAlban\n\n\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 1bee26ebd5..2831abd0fa 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1863,7 +1863,6 @@ static int do_pick_commit(struct repository *r,\n \t\t\t\t       NULL, 0))\n \t\t\treturn error_dirty_index(r, opts);\n \t}\n-\tdiscard_index(r->index);\n \n \tif (!commit->parents)\n \t\tparent = NULL;\n\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 11869ad81c..47f67079f3 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -421,7 +421,7 @@ static int unpack_trees_start(struct merge_options *opt,\n {\n \tint rc;\n \tstruct tree_desc t[3];\n-\tstruct index_state tmp_index = { NULL };\n+\t/* struct index_state tmp_index = { NULL }; */\n \n \tmemset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts));\n \tif (opt->priv->call_depth)\n@@ -432,7 +432,7 @@ static int unpack_trees_start(struct merge_options *opt,\n \topt->priv->unpack_opts.head_idx = 2;\n \topt->priv->unpack_opts.fn = threeway_merge;\n \topt->priv->unpack_opts.src_index = opt->repo->index;\n-\topt->priv->unpack_opts.dst_index = &tmp_index;\n+\topt->priv->unpack_opts.dst_index = opt->repo->index;\n \topt->priv->unpack_opts.aggressive = !merge_detect_rename(opt);\n \tsetup_unpack_trees_porcelain(&opt->priv->unpack_opts, \"merge\");\n \n@@ -449,8 +449,8 @@ static int unpack_trees_start(struct merge_options *opt,\n \t * saved copy.  (verify_uptodate() checks src_index, and the original\n \t * index is the one that had the necessary modification timestamps.)\n \t */\n-\topt->priv->orig_index = *opt->repo->index;\n-\t*opt->repo->index = tmp_index;\n+\t/* opt->priv->orig_index = *opt->repo->index; */\n+\t/* *opt->repo->index = tmp_index; */\n \topt->priv->unpack_opts.src_index = &opt->priv->orig_index;\n \n \treturn rc;\n"},{"id":"389014","messageId":"CABPp-BGCQR+MLVTDoaXDmPrE4SCu+dQ794X6Kvx1PpiQ=6D7KQ@mail.gmail.com","threadId":"50375","inReplyTo":"a00e1689-ec7c-4039-a2e9-f72d452ae4ff@gmail.com","subject":"Re: Comparing rebase --am with --interactive via p3400","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-12-27T22:45:55Z","receivedAt":"2019-12-27T22:46:06Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Alban,\n\nOn Fri, Dec 27, 2019 at 1:11 PM Alban Gruin <alban.gruin@gmail.com> wrote:\n>\n> Hi Johannes & Elijah,\n>\n> Le 01/02/2019 à 07:04, Johannes Schindelin a écrit :\n> > Hi Elijah,\n> >\n> > as discussed at the Contributors' Summit, I ran p3400 as-is (i.e. with the\n> > --am backend) and then with --keep-empty to force the interactive backend\n> > to be used. Here are the best of 10, on my relatively powerful Windows 10\n> > laptop, with current `master`.\n> >\n> > With regular rebase --am:\n> >\n> > 3400.2: rebase on top of a lot of unrelated changes             5.32(0.06+0.15)\n> > 3400.4: rebase a lot of unrelated changes without split-index   33.08(0.04+0.18)\n> > 3400.6: rebase a lot of unrelated changes with split-index      30.29(0.03+0.18)\n> >\n> > with --keep-empty to force the interactive backend:\n> >\n> > 3400.2: rebase on top of a lot of unrelated changes             3.92(0.03+0.18)\n> > 3400.4: rebase a lot of unrelated changes without split-index   33.92(0.03+0.22)\n> > 3400.6: rebase a lot of unrelated changes with split-index      38.82(0.03+0.16)\n> >\n> > I then changed it to -m to test the current scripted version, trying to\n> > let it run overnight, but my laptop eventually went to sleep and the tests\n> > were not even done. I'll let them continue and report back.\n> >\n> > My conclusion after seeing these numbers is: the interactive rebase is\n> > really close to the performance of the --am backend. So to me, it makes a\n> > total lot of sense to switch --merge over to it, and to make --merge the\n> > default. We still should investigate why the split-index performance is so\n> > significantly worse, though.\n> >\n> > Ciao,\n> > Dscho\n> >\n>\n> I investigated a bit on this.  From a quick glance at a callgrind trace,\n> I can see that ce_write_entry() is called 20 601[1] times with `git am',\n> but 739 802 times with the sequencer when the split-index is enabled.\n\nSweet, thanks for digging in and analyzing this.\n\n> For reference, here are the timings, measured on my Linux machine, on a\n> tmpfs, with git.git as the repo:\n>\n> `rebase --am':\n> > 3400.2: rebase on top of a lot of unrelated changes             0.29(0.24+0.03)\n> > 3400.4: rebase a lot of unrelated changes without split-index   6.77(6.51+0.22)\n> > 3400.6: rebase a lot of unrelated changes with split-index      4.43(4.29+0.13)\n> `rebase --quiet':\n\n--quiet?  Isn't that flag supposed to work with both backends and not\nimply either one?  We previously used --keep-empty, though there's a\nchance that flag means we're not doing a fair comparison (since 'am'\nwill drop empty commits and thus have less work to do).  Is there any\nchance you actually ran a different command, but when you went to\nsummarize just typed the wrong flag name?  Anyway, the best would\nprobably be to use --merge here (at the time Johannes and I were\ntesting, that wouldn't have triggered the sequencer, but it does now),\nafter first applying the en/rebase-backend series just to make sure\nwe're doing an apples to apples comparison.  However, I suspect that\nempty commits probably weren't much of a factor and you did find some\ninteresting things...\n\n> > 3400.2: rebase on top of a lot of unrelated changes             0.24(0.21+0.02)\n> > 3400.4: rebase a lot of unrelated changes without split-index   5.60(5.32+0.27)\n> > 3400.6: rebase a lot of unrelated changes with split-index      5.67(5.40+0.26)\n>\n> This comes from two things:\n>\n> 1. There is not enough shared entries in the index with the sequencer.\n>\n> do_write_index() is called only by do_write_locked_index() with `--am',\n> but is also called by write_shared_index() with the sequencer once for\n> every other commit.  As the latter is only called by\n> write_locked_index(), which means that too_many_not_shared_entries()\n> returns true for the sequencer, but never for `--am'.\n>\n> Removing the call to discard_index() in do_pick_commit() (as in the\n> first attached patch) solve this particular issue, but this would\n> require a more thorough analysis to see if it is actually safe to do.\n\nI'm actually surprised the sequencer would call discard_index(); I\nwould have thought it would have relied on merge_recursive() to do the\nnecessary index changes and updates other than writing the new index\nout.  But I'm not quite as familar with the sequencer so perhaps\nthere's some reason I'm unaware of.  (Any chance this is a left-over\nfrom when sequencer invoked external scripts to do the work, and thus\nthe index was updated in another processes' memory and on disk, and it\nhad to discard and re-read to get its own process updated?)\n\n> After this, ce_write() is still called much more by the sequencer.\n>\n> Here are the results of `rebase --quiet' without discarding the index:\n>\n> > 3400.2: rebase on top of a lot of unrelated changes             0.23(0.19+0.04)\n> > 3400.4: rebase a lot of unrelated changes without split-index   5.14(4.95+0.18)\n> > 3400.6: rebase a lot of unrelated changes with split-index      5.02(4.87+0.15)\n> The performance of the rebase is better in the two cases.\n\nNice.  :-)\n\n> 2. The base index is dropped by unpack_trees_start() and unpack_trees().\n>\n> Now, write_shared_index() is no longer called and write_locked_index()\n> is less expensive than before according to callgrind.  But\n> ce_write_entry() is still called 749 302 times (which is even more than\n> before.)\n>\n> The only place where ce_write_entry() is called is in a loop in\n> do_write_index().  The number of iterations is dictated by the size of\n> the cache, and there is a trace2 probe dumping this value.\n>\n> For `--am', the value goes like this: 1, 2, 2, 2, 2, 3, 3, 3, 3, 4, 4,\n> 4, 4, 5, 5, 5, 5, … up until 101.\n>\n> For the sequencer, it goes like this: 1, 1, 3697, 3697, 3698, 3698,\n> 3699, 3699, … up until 3796.\n>\n> The size of the cache is set in prepare_to_write_split_index().  It\n> grows if a cache entry has no index (most of them should have one by\n> now), or if the split index has no base index (with `--am', the split\n> index always has a base.)  This comes from unpack_trees_start() -- it\n> creates a new index, and unpack_trees() does not carry the base index,\n> hence the size of the cache.\n>\n> The second attached patch (which is broken for the non-interactive\n> rebase case) demonstrates what we could expect for the split-index case\n> if we fix this:\n>\n> > 3400.2: rebase on top of a lot of unrelated changes             0.24(0.21+0.03)\n> > 3400.4: rebase a lot of unrelated changes without split-index   5.81(5.62+0.17)\n> > 3400.6: rebase a lot of unrelated changes with split-index      4.76(4.54+0.20)\n> So, for everything related to the index, I think that’s it.\n>\n> [1] Numbers may vary, but they should remain in the same order of magnitude.\n\nUnfortunately, this patch as-is breaks some important things even if\nit only shows up in a few testcases.  merge-recursive needs to know\nboth what the index looked like before the merge started, as well as\nwhat it looks like after unpack-trees runs; see commits 1de70dbd1a\n(merge-recursive: fix check for skipability of working tree updates,\n2018-04-19) and a35edc84bd (merge-recursive: fix was_tracked() to quit\nlying with some renamed paths, 2018-04-19), and maybe a few others\nfrom that series.\n\nBut, noting that it comes from the differences in the index as\nunpack_trees runs is useful info.  I might be restructuring this code\nsomewhat significantly but it helps to have this in mind; I may spot\nopportunities to do something with it while I'm digging in...\n\nElijah\n"},{"id":"389038","messageId":"16add63e-a631-5ebf-bbbe-17823d942ee9@gmail.com","threadId":"50375","inReplyTo":"CABPp-BGCQR+MLVTDoaXDmPrE4SCu+dQ794X6Kvx1PpiQ=6D7KQ@mail.gmail.com","subject":"Re: Comparing rebase --am with --interactive via p3400","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2019-12-29T17:25:14Z","receivedAt":"2019-12-29T18:15:31Z","isPatch":false,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Elijah,\n\nLe 27/12/2019 à 23:45, Elijah Newren a écrit :\n> Hi Alban,\n> \n> On Fri, Dec 27, 2019 at 1:11 PM Alban Gruin <alban.gruin@gmail.com> wrote:\n>>\n>> Hi Johannes & Elijah,\n>>\n>> Le 01/02/2019 à 07:04, Johannes Schindelin a écrit :\n>>> Hi Elijah,\n>>>\n>>> as discussed at the Contributors' Summit, I ran p3400 as-is (i.e. with the\n>>> --am backend) and then with --keep-empty to force the interactive backend\n>>> to be used. Here are the best of 10, on my relatively powerful Windows 10\n>>> laptop, with current `master`.\n>>>\n>>> With regular rebase --am:\n>>>\n>>> 3400.2: rebase on top of a lot of unrelated changes             5.32(0.06+0.15)\n>>> 3400.4: rebase a lot of unrelated changes without split-index   33.08(0.04+0.18)\n>>> 3400.6: rebase a lot of unrelated changes with split-index      30.29(0.03+0.18)\n>>>\n>>> with --keep-empty to force the interactive backend:\n>>>\n>>> 3400.2: rebase on top of a lot of unrelated changes             3.92(0.03+0.18)\n>>> 3400.4: rebase a lot of unrelated changes without split-index   33.92(0.03+0.22)\n>>> 3400.6: rebase a lot of unrelated changes with split-index      38.82(0.03+0.16)\n>>>\n>>> I then changed it to -m to test the current scripted version, trying to\n>>> let it run overnight, but my laptop eventually went to sleep and the tests\n>>> were not even done. I'll let them continue and report back.\n>>>\n>>> My conclusion after seeing these numbers is: the interactive rebase is\n>>> really close to the performance of the --am backend. So to me, it makes a\n>>> total lot of sense to switch --merge over to it, and to make --merge the\n>>> default. We still should investigate why the split-index performance is so\n>>> significantly worse, though.\n>>>\n>>> Ciao,\n>>> Dscho\n>>>\n>>\n>> I investigated a bit on this.  From a quick glance at a callgrind trace,\n>> I can see that ce_write_entry() is called 20 601[1] times with `git am',\n>> but 739 802 times with the sequencer when the split-index is enabled.\n> \n> Sweet, thanks for digging in and analyzing this.\n> \n>> For reference, here are the timings, measured on my Linux machine, on a\n>> tmpfs, with git.git as the repo:\n>>\n>> `rebase --am':\n>>> 3400.2: rebase on top of a lot of unrelated changes             0.29(0.24+0.03)\n>>> 3400.4: rebase a lot of unrelated changes without split-index   6.77(6.51+0.22)\n>>> 3400.6: rebase a lot of unrelated changes with split-index      4.43(4.29+0.13)\n>> `rebase --quiet':\n> \n> --quiet?  Isn't that flag supposed to work with both backends and not\n> imply either one?  We previously used --keep-empty, though there's a\n> chance that flag means we're not doing a fair comparison (since 'am'\n> will drop empty commits and thus have less work to do).  Is there any\n> chance you actually ran a different command, but when you went to\n> summarize just typed the wrong flag name?  Anyway, the best would\n> probably be to use --merge here (at the time Johannes and I were\n> testing, that wouldn't have triggered the sequencer, but it does now),\n> after first applying the en/rebase-backend series just to make sure\n> we're doing an apples to apples comparison.  However, I suspect that\n> empty commits probably weren't much of a factor and you did find some\n> interesting things...\n> \n\nYes, I did use `--keep-empty' but misremembered it when writing this email…\n\n>>> 3400.2: rebase on top of a lot of unrelated changes             0.24(0.21+0.02)\n>>> 3400.4: rebase a lot of unrelated changes without split-index   5.60(5.32+0.27)\n>>> 3400.6: rebase a lot of unrelated changes with split-index      5.67(5.40+0.26)\n>>\n>> This comes from two things:\n>>\n>> 1. There is not enough shared entries in the index with the sequencer.\n>>\n>> do_write_index() is called only by do_write_locked_index() with `--am',\n>> but is also called by write_shared_index() with the sequencer once for\n>> every other commit.  As the latter is only called by\n>> write_locked_index(), which means that too_many_not_shared_entries()\n>> returns true for the sequencer, but never for `--am'.\n>>\n>> Removing the call to discard_index() in do_pick_commit() (as in the\n>> first attached patch) solve this particular issue, but this would\n>> require a more thorough analysis to see if it is actually safe to do.\n> \n> I'm actually surprised the sequencer would call discard_index(); I\n> would have thought it would have relied on merge_recursive() to do the\n> necessary index changes and updates other than writing the new index\n> out.  But I'm not quite as familar with the sequencer so perhaps\n> there's some reason I'm unaware of.  (Any chance this is a left-over\n> from when sequencer invoked external scripts to do the work, and thus\n> the index was updated in another processes' memory and on disk, and it\n> had to discard and re-read to get its own process updated?)\n> \n\nThe sequencer re-reads the index after invoking an external command\n(either `git checkout', `git merge' or an `exec' command from the todo\nlist), which makes sense.  But this one seems to come from 6eb1b437933\n(\"cherry-pick/revert: make direct internal call to merge_tree()\",\n2008-09-02).  So, yes, quite old, and perhaps no longer justified.\n\nI know I had to add another discard_cache() in rebase--interactive.c\nbecause it broke something with the submodules, but it does not seems\nall that useful now that rebase.c no longer has to fork to use the\nsequencer.\n\nCheers,\nAlban\n\n"},{"id":"389176","messageId":"nycvar.QRO.7.76.6.2001022108280.46@tvgsbejvaqbjf.bet","threadId":"50375","inReplyTo":"16add63e-a631-5ebf-bbbe-17823d942ee9@gmail.com","subject":"Re: Comparing rebase --am with --interactive via p3400","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-02T20:17:36Z","receivedAt":"2020-01-02T20:17:57Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Alban & Elijah,\n\nOn Sun, 29 Dec 2019, Alban Gruin wrote:\n\n> Hi Elijah,\n>\n> Le 27/12/2019 à 23:45, Elijah Newren a écrit :\n> > Hi Alban,\n> >\n> > On Fri, Dec 27, 2019 at 1:11 PM Alban Gruin <alban.gruin@gmail.com> wrote:\n> >>\n> >> Hi Johannes & Elijah,\n> >>\n> >> Le 01/02/2019 à 07:04, Johannes Schindelin a écrit :\n> >>> Hi Elijah,\n> >>>\n> >>> as discussed at the Contributors' Summit, I ran p3400 as-is (i.e. with the\n> >>> --am backend) and then with --keep-empty to force the interactive backend\n> >>> to be used. Here are the best of 10, on my relatively powerful Windows 10\n> >>> laptop, with current `master`.\n> >>>\n> >>> With regular rebase --am:\n> >>>\n> >>> 3400.2: rebase on top of a lot of unrelated changes             5.32(0.06+0.15)\n> >>> 3400.4: rebase a lot of unrelated changes without split-index   33.08(0.04+0.18)\n> >>> 3400.6: rebase a lot of unrelated changes with split-index      30.29(0.03+0.18)\n> >>>\n> >>> with --keep-empty to force the interactive backend:\n> >>>\n> >>> 3400.2: rebase on top of a lot of unrelated changes             3.92(0.03+0.18)\n> >>> 3400.4: rebase a lot of unrelated changes without split-index   33.92(0.03+0.22)\n> >>> 3400.6: rebase a lot of unrelated changes with split-index      38.82(0.03+0.16)\n> >>>\n> >>> I then changed it to -m to test the current scripted version, trying to\n> >>> let it run overnight, but my laptop eventually went to sleep and the tests\n> >>> were not even done. I'll let them continue and report back.\n> >>>\n> >>> My conclusion after seeing these numbers is: the interactive rebase is\n> >>> really close to the performance of the --am backend. So to me, it makes a\n> >>> total lot of sense to switch --merge over to it, and to make --merge the\n> >>> default. We still should investigate why the split-index performance is so\n> >>> significantly worse, though.\n> >>>\n> >>> Ciao,\n> >>> Dscho\n> >>>\n> >>\n> >> I investigated a bit on this.  From a quick glance at a callgrind trace,\n> >> I can see that ce_write_entry() is called 20 601[1] times with `git am',\n> >> but 739 802 times with the sequencer when the split-index is enabled.\n> >\n> > Sweet, thanks for digging in and analyzing this.\n> >\n> >> For reference, here are the timings, measured on my Linux machine, on a\n> >> tmpfs, with git.git as the repo:\n> >>\n> >> `rebase --am':\n> >>> 3400.2: rebase on top of a lot of unrelated changes             0.29(0.24+0.03)\n> >>> 3400.4: rebase a lot of unrelated changes without split-index   6.77(6.51+0.22)\n> >>> 3400.6: rebase a lot of unrelated changes with split-index      4.43(4.29+0.13)\n> >> `rebase --quiet':\n> >\n> > --quiet?  Isn't that flag supposed to work with both backends and not\n> > imply either one?  We previously used --keep-empty, though there's a\n> > chance that flag means we're not doing a fair comparison (since 'am'\n> > will drop empty commits and thus have less work to do).  Is there any\n> > chance you actually ran a different command, but when you went to\n> > summarize just typed the wrong flag name?  Anyway, the best would\n> > probably be to use --merge here (at the time Johannes and I were\n> > testing, that wouldn't have triggered the sequencer, but it does now),\n> > after first applying the en/rebase-backend series just to make sure\n> > we're doing an apples to apples comparison.  However, I suspect that\n> > empty commits probably weren't much of a factor and you did find some\n> > interesting things...\n> >\n>\n> Yes, I did use `--keep-empty' but misremembered it when writing this email…\n>\n> >>> 3400.2: rebase on top of a lot of unrelated changes             0.24(0.21+0.02)\n> >>> 3400.4: rebase a lot of unrelated changes without split-index   5.60(5.32+0.27)\n> >>> 3400.6: rebase a lot of unrelated changes with split-index      5.67(5.40+0.26)\n> >>\n> >> This comes from two things:\n> >>\n> >> 1. There is not enough shared entries in the index with the sequencer.\n> >>\n> >> do_write_index() is called only by do_write_locked_index() with `--am',\n> >> but is also called by write_shared_index() with the sequencer once for\n> >> every other commit.  As the latter is only called by\n> >> write_locked_index(), which means that too_many_not_shared_entries()\n> >> returns true for the sequencer, but never for `--am'.\n> >>\n> >> Removing the call to discard_index() in do_pick_commit() (as in the\n> >> first attached patch) solve this particular issue, but this would\n> >> require a more thorough analysis to see if it is actually safe to do.\n> >\n> > I'm actually surprised the sequencer would call discard_index(); I\n> > would have thought it would have relied on merge_recursive() to do the\n> > necessary index changes and updates other than writing the new index\n> > out.  But I'm not quite as familar with the sequencer so perhaps\n> > there's some reason I'm unaware of.  (Any chance this is a left-over\n> > from when sequencer invoked external scripts to do the work, and thus\n> > the index was updated in another processes' memory and on disk, and it\n> > had to discard and re-read to get its own process updated?)\n> >\n>\n> The sequencer re-reads the index after invoking an external command\n> (either `git checkout', `git merge' or an `exec' command from the todo\n> list), which makes sense.  But this one seems to come from 6eb1b437933\n> (\"cherry-pick/revert: make direct internal call to merge_tree()\",\n> 2008-09-02).  So, yes, quite old, and perhaps no longer justified.\n\nRight. This commit also moved the `discard_cache()` call outside from the\n`else` clause of the `if (no_commit)`.\n\nThat `else` clause goes all the way back to 9509af686bf (Make git-revert &\ngit-cherry-pick a builtin, 2007-03-01), and I admit freely that my memory\nis no longer fresh on the specifics of this patch.\n\nLooking at that patch, I think I simply discarded the index because a\nsubsequent code path would spawn the `git merge-recursive` process, which\nwould have changed the index externally.\n\n> I know I had to add another discard_cache() in rebase--interactive.c\n> because it broke something with the submodules, but it does not seems\n> all that useful now that rebase.c no longer has to fork to use the\n> sequencer.\n\nFWIW I agree. The code is still quite complex at this point, but\ninfinitely more readable (thank you Elijah for taking point on simplifying\nmerge-recursive.c!). So I think that it might be the right point in time\nto make sure that the index is not re-read and re-discarded over and over\nagain.\n\nThanks,\nDscho\n"},{"id":"390965","messageId":"nycvar.QRO.7.76.6.2001312216410.46@tvgsbejvaqbjf.bet","threadId":"50375","inReplyTo":"a00e1689-ec7c-4039-a2e9-f72d452ae4ff@gmail.com","subject":"Re: Comparing rebase --am with --interactive via p3400","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-31T21:23:33Z","receivedAt":"2020-01-31T21:23:39Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Alban,\n\nOn Fri, 27 Dec 2019, Alban Gruin wrote:\n\n> Le 01/02/2019 à 07:04, Johannes Schindelin a écrit :\n>\n> > as discussed at the Contributors' Summit, I ran p3400 as-is (i.e. with the\n> > --am backend) and then with --keep-empty to force the interactive backend\n> > to be used. Here are the best of 10, on my relatively powerful Windows 10\n> > laptop, with current `master`.\n> >\n> > With regular rebase --am:\n> >\n> > 3400.2: rebase on top of a lot of unrelated changes             5.32(0.06+0.15)\n> > 3400.4: rebase a lot of unrelated changes without split-index   33.08(0.04+0.18)\n> > 3400.6: rebase a lot of unrelated changes with split-index      30.29(0.03+0.18)\n> >\n> > with --keep-empty to force the interactive backend:\n> >\n> > 3400.2: rebase on top of a lot of unrelated changes             3.92(0.03+0.18)\n> > 3400.4: rebase a lot of unrelated changes without split-index   33.92(0.03+0.22)\n> > 3400.6: rebase a lot of unrelated changes with split-index      38.82(0.03+0.16)\n> >\n> > I then changed it to -m to test the current scripted version, trying to\n> > let it run overnight, but my laptop eventually went to sleep and the tests\n> > were not even done. I'll let them continue and report back.\n> >\n> > My conclusion after seeing these numbers is: the interactive rebase is\n> > really close to the performance of the --am backend. So to me, it makes a\n> > total lot of sense to switch --merge over to it, and to make --merge the\n> > default. We still should investigate why the split-index performance is so\n> > significantly worse, though.\n> >\n> > Ciao,\n> > Dscho\n> >\n>\n> I investigated a bit on this.  From a quick glance at a callgrind trace,\n> I can see that ce_write_entry() is called 20 601[1] times with `git am',\n> but 739 802 times with the sequencer when the split-index is enabled.\n>\n> For reference, here are the timings, measured on my Linux machine, on a\n> tmpfs, with git.git as the repo:\n>\n> `rebase --am':\n> > 3400.2: rebase on top of a lot of unrelated changes             0.29(0.24+0.03)\n> > 3400.4: rebase a lot of unrelated changes without split-index   6.77(6.51+0.22)\n> > 3400.6: rebase a lot of unrelated changes with split-index      4.43(4.29+0.13)\n> `rebase --quiet':\n> > 3400.2: rebase on top of a lot of unrelated changes             0.24(0.21+0.02)\n> > 3400.4: rebase a lot of unrelated changes without split-index   5.60(5.32+0.27)\n> > 3400.6: rebase a lot of unrelated changes with split-index      5.67(5.40+0.26)\n>\n> This comes from two things:\n>\n> 1. There is not enough shared entries in the index with the sequencer.\n>\n> do_write_index() is called only by do_write_locked_index() with `--am',\n> but is also called by write_shared_index() with the sequencer once for\n> every other commit.  As the latter is only called by\n> write_locked_index(), which means that too_many_not_shared_entries()\n> returns true for the sequencer, but never for `--am'.\n>\n> Removing the call to discard_index() in do_pick_commit() (as in the\n> first attached patch) solve this particular issue, but this would\n> require a more thorough analysis to see if it is actually safe to do.\n\nIndeed. I offered these insights in #git-devel (slightly edited):\n\nThis `discard_index()` is in an awfully central location. I am rather\ncertain that it would cause problems to just remove it.\n\nLooking at `do_merge()`: it explicitly discards and re-reads the index if\nwe had to spawn a `git merge` process (which we do if a strategy other\nthan `recursive` was specified, or if it is an octopus merge). But I am\nwary of other code paths that might not be as careful.\n\nI see that `do_exec()` is similarly careful.\n\nOne thing I cannot fail to notice: we do not re-read a changed index\nafter running the `prepare-commit-msg` hook, or for that matter, any other\nhook. That could even be an old regression from the conversion of the\ninteractive rebase to using the sequencer rather than a shell script.\n\nFurther, `reset_merge()` seems to spawn `git reset --merge` without\nbothering to re-read the possibly modified index. Its callees are\n`rollback_single_pick()`, `skip_single_pick()` and `sequencer_rollback()`,\nnone of which seem to be careful, either, about checking whether the index\nwas modified in the meantime.\n\nTechnically, the in-memory index should also be discarded\nin `apply_autostash()`, but so far, we do not have any callers of that\nfunction, I don't think, that wants to do anything but release resources\nand exit.\n\nThe `run_git_checkout()` function discards, as intended. I\nam not quite sure whether it needs to, though, unless the `.git/index`\nfile _was_ modified (it _is_ possible, after all, to run `git rebase -i\nHEAD`, and I do have a use case for that where one of my scripts generates\na todo script, sort of a `git cherry-pick --rebase-merges`, because\n`cherry-pick` does not support that mode).\n\nThe `continue_single_pick()` function spawns a `git\ncommit` which could potentially modify the index through a hook, but the\nfirst call site does not care and the second one guards against that\n(erroring out...).\n\nMy biggest concern is with the `run_git_commit()` function: it does not\nre-read a potentially-modified index (think of hooks).\n\nWe will need to be very careful with this `discard_index()`, I think, and\nin my opinion there is a great opportunity here for cleaning up a little:\nrather than discarding and re-reading the in-memory index without seeing\nwhether the on-disk index has changed at all appears a bit wasteful to me.\n\nThis could be refactored into a function that only discards and re-reads\nthe index if the mtime of `.git/index` changed. That function could then\nalso be taught to detect when the in-memory index has unwritten changes:\nthat would constitute a bug.\n\nCiao,\nDscho\n\n>\n> After this, ce_write() is still called much more by the sequencer.\n>\n> Here are the results of `rebase --quiet' without discarding the index:\n>\n> > 3400.2: rebase on top of a lot of unrelated changes             0.23(0.19+0.04)\n> > 3400.4: rebase a lot of unrelated changes without split-index   5.14(4.95+0.18)\n> > 3400.6: rebase a lot of unrelated changes with split-index      5.02(4.87+0.15)\n> The performance of the rebase is better in the two cases.\n>\n>\n> 2. The base index is dropped by unpack_trees_start() and unpack_trees().\n>\n> Now, write_shared_index() is no longer called and write_locked_index()\n> is less expensive than before according to callgrind.  But\n> ce_write_entry() is still called 749 302 times (which is even more than\n> before.)\n>\n> The only place where ce_write_entry() is called is in a loop in\n> do_write_index().  The number of iterations is dictated by the size of\n> the cache, and there is a trace2 probe dumping this value.\n>\n> For `--am', the value goes like this: 1, 2, 2, 2, 2, 3, 3, 3, 3, 4, 4,\n> 4, 4, 5, 5, 5, 5, … up until 101.\n>\n> For the sequencer, it goes like this: 1, 1, 3697, 3697, 3698, 3698,\n> 3699, 3699, … up until 3796.\n>\n> The size of the cache is set in prepare_to_write_split_index().  It\n> grows if a cache entry has no index (most of them should have one by\n> now), or if the split index has no base index (with `--am', the split\n> index always has a base.)  This comes from unpack_trees_start() -- it\n> creates a new index, and unpack_trees() does not carry the base index,\n> hence the size of the cache.\n>\n> The second attached patch (which is broken for the non-interactive\n> rebase case) demonstrates what we could expect for the split-index case\n> if we fix this:\n>\n> > 3400.2: rebase on top of a lot of unrelated changes             0.24(0.21+0.03)\n> > 3400.4: rebase a lot of unrelated changes without split-index   5.81(5.62+0.17)\n> > 3400.6: rebase a lot of unrelated changes with split-index      4.76(4.54+0.20)\n> So, for everything related to the index, I think that’s it.\n>\n> [1] Numbers may vary, but they should remain in the same order of magnitude.\n>\n> Cheers,\n> Alban\n>\n>\n"},{"id":"394487","messageId":"b380123e-dbcd-7e92-67ab-e6cbf0cec061@gmail.com","threadId":"50375","inReplyTo":"nycvar.QRO.7.76.6.2001312216410.46@tvgsbejvaqbjf.bet","subject":"Re: Comparing rebase --am with --interactive via p3400","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2020-04-01T11:33:27Z","receivedAt":"2020-04-01T11:33:53Z","isPatch":false,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Johannes,\n\nSorry for the late answer, I was really busy for the last months.\n\nLe 31/01/2020 à 22:23, Johannes Schindelin a écrit :\n> Hi Alban,\n>  -%<-\n> \n> Indeed. I offered these insights in #git-devel (slightly edited):\n> \n> This `discard_index()` is in an awfully central location. I am rather\n> certain that it would cause problems to just remove it.\n> \n> Looking at `do_merge()`: it explicitly discards and re-reads the index if\n> we had to spawn a `git merge` process (which we do if a strategy other\n> than `recursive` was specified, or if it is an octopus merge). But I am\n> wary of other code paths that might not be as careful.\n> \n> I see that `do_exec()` is similarly careful.\n> \n\nI have to admit that the index is not my area of expertise in git, so\nsorry if my question is stupid: isn't there a less heavy way to find\nunstaged or uncommitted changes than discarding and then reloading the\nindex?\n\n> One thing I cannot fail to notice: we do not re-read a changed index\n> after running the `prepare-commit-msg` hook, or for that matter, any other\n> hook. That could even be an old regression from the conversion of the\n> interactive rebase to using the sequencer rather than a shell script.\n> \n> Further, `reset_merge()` seems to spawn `git reset --merge` without\n> bothering to re-read the possibly modified index. Its callees are\n> `rollback_single_pick()`, `skip_single_pick()` and `sequencer_rollback()`,\n> none of which seem to be careful, either, about checking whether the index\n> was modified in the meantime.\n> \n> Technically, the in-memory index should also be discarded\n> in `apply_autostash()`, but so far, we do not have any callers of that\n> function, I don't think, that wants to do anything but release resources\n> and exit.\n> \n> The `run_git_checkout()` function discards, as intended. I\n> am not quite sure whether it needs to, though, unless the `.git/index`\n> file _was_ modified (it _is_ possible, after all, to run `git rebase -i\n> HEAD`, and I do have a use case for that where one of my scripts generates\n> a todo script, sort of a `git cherry-pick --rebase-merges`, because\n> `cherry-pick` does not support that mode).\n> \n> The `continue_single_pick()` function spawns a `git\n> commit` which could potentially modify the index through a hook, but the\n> first call site does not care and the second one guards against that\n> (erroring out...).\n> \n> My biggest concern is with the `run_git_commit()` function: it does not\n> re-read a potentially-modified index (think of hooks).\n\nThank you for your analysis.\n\n> \n> We will need to be very careful with this `discard_index()`, I think, and\n> in my opinion there is a great opportunity here for cleaning up a little:\n> rather than discarding and re-reading the in-memory index without seeing\n> whether the on-disk index has changed at all appears a bit wasteful to me.\n> \n> This could be refactored into a function that only discards and re-reads\n> the index if the mtime of `.git/index` changed. That function could then\n> also be taught to detect when the in-memory index has unwritten changes:\n> that would constitute a bug.\n> \n\nHmm, checking if the mtime of the index to see if it changed isn't racy?\n Sub-second changes should happen, and to quote a comment in\nis_racy_stat(), \"nanosecond timestamped files can also be racy\" -- even\nif it should not really happen in the case of rebase…\n\n> Ciao,\n> Dscho\n> \n\nAlban\n\n"},{"id":"394494","messageId":"a92f1028-d33b-bd7e-a4d5-f4884faedeed@gmail.com","threadId":"50375","inReplyTo":"b380123e-dbcd-7e92-67ab-e6cbf0cec061@gmail.com","subject":"Re: Comparing rebase --am with --interactive via p3400","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-04-01T14:00:19Z","receivedAt":"2020-04-01T14:00:26Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alban and Johannes\n\nOn 01/04/2020 12:33, Alban Gruin wrote:\n> Hi Johannes,\n> \n> Sorry for the late answer, I was really busy for the last months.\n> \n> Le 31/01/2020 à 22:23, Johannes Schindelin a écrit :\n>> Hi Alban,\n>>   -%<-\n>>\n>> Indeed. I offered these insights in #git-devel (slightly edited):\n>>\n>> This `discard_index()` is in an awfully central location. I am rather\n>> certain that it would cause problems to just remove it.\n>>\n>> Looking at `do_merge()`: it explicitly discards and re-reads the index if\n>> we had to spawn a `git merge` process (which we do if a strategy other\n>> than `recursive` was specified, or if it is an octopus merge). But I am\n>> wary of other code paths that might not be as careful.\n>>\n>> I see that `do_exec()` is similarly careful.\n>>\n> \n> I have to admit that the index is not my area of expertise in git, so\n> sorry if my question is stupid: isn't there a less heavy way to find\n> unstaged or uncommitted changes than discarding and then reloading the\n> index?\n> \n>> One thing I cannot fail to notice: we do not re-read a changed index\n>> after running the `prepare-commit-msg` hook, or for that matter, any other\n>> hook. That could even be an old regression from the conversion of the\n>> interactive rebase to using the sequencer rather than a shell script.\n>>\n>> Further, `reset_merge()` seems to spawn `git reset --merge` without\n>> bothering to re-read the possibly modified index. Its callees are\n>> `rollback_single_pick()`, `skip_single_pick()` and `sequencer_rollback()`,\n>> none of which seem to be careful, either, about checking whether the index\n>> was modified in the meantime.\n>>\n>> Technically, the in-memory index should also be discarded\n>> in `apply_autostash()`, but so far, we do not have any callers of that\n>> function, I don't think, that wants to do anything but release resources\n>> and exit.\n>>\n>> The `run_git_checkout()` function discards, as intended. I\n>> am not quite sure whether it needs to, though, unless the `.git/index`\n>> file _was_ modified (it _is_ possible, after all, to run `git rebase -i\n>> HEAD`, and I do have a use case for that where one of my scripts generates\n>> a todo script, sort of a `git cherry-pick --rebase-merges`, because\n>> `cherry-pick` does not support that mode).\n\nI'm not sure it is worth optimizing the case where .git/index is not \nchanged as we only do this once per rebase. In any case I hope that one \nday we'll stop forking git checkout and use the code from \nbuiltin/rebase.c to do it\n\n>> The `continue_single_pick()` function spawns a `git\n>> commit` which could potentially modify the index through a hook, but the\n>> first call site does not care and the second one guards against that\n>> (erroring out...).\n>>\n>> My biggest concern is with the `run_git_commit()` function: it does not\n>> re-read a potentially-modified index (think of hooks).\n\nI agree that we should be re-reading the index after forking `git \ncommit` and also `git merge`. Most of the time we commit without forking \nso that should not impact the performance too much\n\n> Thank you for your analysis.\n> \n>>\n>> We will need to be very careful with this `discard_index()`, I think, and\n>> in my opinion there is a great opportunity here for cleaning up a little:\n>> rather than discarding and re-reading the in-memory index without seeing\n>> whether the on-disk index has changed at all appears a bit wasteful to me.\n>>\n>> This could be refactored into a function that only discards and re-reads\n>> the index if the mtime of `.git/index` changed. That function could then\n>> also be taught to detect when the in-memory index has unwritten changes:\n>> that would constitute a bug.\n>>\n> \n> Hmm, checking if the mtime of the index to see if it changed isn't racy?\n>   Sub-second changes should happen, and to quote a comment in\n> is_racy_stat(), \"nanosecond timestamped files can also be racy\" -- even\n> if it should not really happen in the case of rebase…\n\nI don't think relying on the index stat data is a good idea, git \ndefaults to one second mtime resolution unless it is built with \n-DUSE_NSEC and we do way more than one commit a second. We tried to rely \non stat data to determine when to re-read the todo list after an exec \nand it is broken (both in the design because it assumes ns mtime \nresolution and the implementation because we don't update the cached \nmtime after we rewrite the todo list). There are not that many places \nwhere we need to re-read the index so I think we should just have \nexplicit re-reads where we need them. Hopefully over time we'll stop \nforking other processes and the problem will go away.\n\nBest Wishes\n\nPhillip\n\n>> Ciao,\n>> Dscho\n>>\n> \n> Alban\n> \n"},{"id":"394783","messageId":"nycvar.QRO.7.76.6.2004042232140.46@tvgsbejvaqbjf.bet","threadId":"50375","inReplyTo":"a92f1028-d33b-bd7e-a4d5-f4884faedeed@gmail.com","subject":"Re: Comparing rebase --am with --interactive via p3400","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-04-04T20:33:44Z","receivedAt":"2020-04-04T20:33:55Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Wed, 1 Apr 2020, Phillip Wood wrote:\n\n> On 01/04/2020 12:33, Alban Gruin wrote:\n> > Hi Johannes,\n> >\n> > Sorry for the late answer, I was really busy for the last months.\n> >\n> > Le 31/01/2020 à 22:23, Johannes Schindelin a écrit :\n> > > Hi Alban,\n> > >   -%<-\n> > >\n> > > Indeed. I offered these insights in #git-devel (slightly edited):\n> > >\n> > > This `discard_index()` is in an awfully central location. I am rather\n> > > certain that it would cause problems to just remove it.\n> > >\n> > > Looking at `do_merge()`: it explicitly discards and re-reads the index if\n> > > we had to spawn a `git merge` process (which we do if a strategy other\n> > > than `recursive` was specified, or if it is an octopus merge). But I am\n> > > wary of other code paths that might not be as careful.\n> > >\n> > > I see that `do_exec()` is similarly careful.\n> > >\n> >\n> > I have to admit that the index is not my area of expertise in git, so\n> > sorry if my question is stupid: isn't there a less heavy way to find\n> > unstaged or uncommitted changes than discarding and then reloading the\n> > index?\n> >\n> > > One thing I cannot fail to notice: we do not re-read a changed index\n> > > after running the `prepare-commit-msg` hook, or for that matter, any other\n> > > hook. That could even be an old regression from the conversion of the\n> > > interactive rebase to using the sequencer rather than a shell script.\n> > >\n> > > Further, `reset_merge()` seems to spawn `git reset --merge` without\n> > > bothering to re-read the possibly modified index. Its callees are\n> > > `rollback_single_pick()`, `skip_single_pick()` and `sequencer_rollback()`,\n> > > none of which seem to be careful, either, about checking whether the index\n> > > was modified in the meantime.\n> > >\n> > > Technically, the in-memory index should also be discarded\n> > > in `apply_autostash()`, but so far, we do not have any callers of that\n> > > function, I don't think, that wants to do anything but release resources\n> > > and exit.\n> > >\n> > > The `run_git_checkout()` function discards, as intended. I\n> > > am not quite sure whether it needs to, though, unless the `.git/index`\n> > > file _was_ modified (it _is_ possible, after all, to run `git rebase -i\n> > > HEAD`, and I do have a use case for that where one of my scripts generates\n> > > a todo script, sort of a `git cherry-pick --rebase-merges`, because\n> > > `cherry-pick` does not support that mode).\n>\n> I'm not sure it is worth optimizing the case where .git/index is not changed\n> as we only do this once per rebase. In any case I hope that one day we'll stop\n> forking git checkout and use the code from builtin/rebase.c to do it\n>\n> > > The `continue_single_pick()` function spawns a `git\n> > > commit` which could potentially modify the index through a hook, but the\n> > > first call site does not care and the second one guards against that\n> > > (erroring out...).\n> > >\n> > > My biggest concern is with the `run_git_commit()` function: it does not\n> > > re-read a potentially-modified index (think of hooks).\n>\n> I agree that we should be re-reading the index after forking `git commit` and\n> also `git merge`. Most of the time we commit without forking so that should\n> not impact the performance too much\n>\n> > Thank you for your analysis.\n> >\n> > >\n> > > We will need to be very careful with this `discard_index()`, I think, and\n> > > in my opinion there is a great opportunity here for cleaning up a little:\n> > > rather than discarding and re-reading the in-memory index without seeing\n> > > whether the on-disk index has changed at all appears a bit wasteful to me.\n> > >\n> > > This could be refactored into a function that only discards and re-reads\n> > > the index if the mtime of `.git/index` changed. That function could then\n> > > also be taught to detect when the in-memory index has unwritten changes:\n> > > that would constitute a bug.\n> > >\n> >\n> > Hmm, checking if the mtime of the index to see if it changed isn't racy?\n> >   Sub-second changes should happen, and to quote a comment in\n> > is_racy_stat(), \"nanosecond timestamped files can also be racy\" -- even\n> > if it should not really happen in the case of rebase…\n>\n> I don't think relying on the index stat data is a good idea, git defaults to\n> one second mtime resolution unless it is built with -DUSE_NSEC and we do way\n> more than one commit a second. We tried to rely on stat data to determine when\n> to re-read the todo list after an exec and it is broken (both in the design\n> because it assumes ns mtime resolution and the implementation because we don't\n> update the cached mtime after we rewrite the todo list). There are not that\n> many places where we need to re-read the index so I think we should just have\n> explicit re-reads where we need them. Hopefully over time we'll stop forking\n> other processes and the problem will go away.\n\nWell. Even the 1-second granularity should buy us some performance if we\nassume that `same mtime` == `racy`. That should still catch the majority\nof the cases where the index was simply not changed, at least in the\n`do_exec()` case.\n\nCiao,\nDscho\n\n>\n> Best Wishes\n>\n> Phillip\n>\n> > > Ciao,\n> > > Dscho\n> > >\n> >\n> > Alban\n> >\n>\n"}]}