{"thread":{"id":"57954","subject":"[BUG?] Major performance issue with some commands on our repo's master branch","startedAt":"2022-06-04T08:29:24Z","lastAt":"2022-06-09T20:06:55Z","messageCount":12,"participants":["Tassilo Horn","Tao Klerks","Jeff King","Kyle Meyer","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"456655","messageId":"87h750q1b9.fsf@gnu.org","threadId":"57954","inReplyTo":null,"subject":"[BUG?] Major performance issue with some commands on our repo's master branch","fromName":"Tassilo Horn","fromEmail":"tsdh@gnu.org","sentAt":"2022-06-04T07:39:35Z","receivedAt":"2022-06-04T08:29:24Z","isPatch":false,"sender":{"key":"tsdh@gnu.org","avatar":"https://avatars.githubusercontent.com/u/103854?v=4"},"body":"Hi all,\n\n[spoiler alert: I've figured out the config option causing the problem\nwhile writing this long mail, so you might jump straight to the SOLUTION\nsection at the bottom of this mail.]\n\nat my day job, I work on a git repo (sadly non-public, proprietary) with\nthese stats:\n\n- master has about 150000 commits, the last release branch I've also benchmarked above has 144000 commits\n- the history dates back to 2001\n- .git/ is about 1.8 GB\n\nSo it's quite big but not unusually big when compared to linux or other\nfree software projects.\n\nThe typical git commands I use (status, fetch, pull, commit, push,\nrebase, ...) are all quick.  However, I use the git porcelain Magit [1]\nwhich invokes several plumbing commands in order to present to the user\nan always up-to-date extended status buffer of the currently checked out\nbranch showing the current branch.  Some of those plumbing commands are\nextremely slow for no obvious reasons.  The most outstanding command I\ncould pinpoint is this:\n\n--8<---------------cut here---------------start------------->8---\n❯ time git show --no-patch --format=\"%h %s\" \"master^{commit}\" --\n6192a0cfdc6 Merge remote-tracking branch 'origin/SHD_ECORO_3_9_7'\n\n________________________________________________________\nExecuted in   13.21 secs    fish           external\n   usr time   12.99 secs  462.00 micros   12.99 secs\n   sys time    0.17 secs  119.00 micros    0.17 secs\n--8<---------------cut here---------------end--------------->8---\n\nThe interesting thing is that I have this problem only with the master\nbranch.  When I run it for the last release branch, I get these times:\n\n--8<---------------cut here---------------start------------->8---\n❯ time git show --no-patch --format=\"%h %s\" \"SHD_ECORO_3_9_7^{commit}\" --\n994334fc9fb ECOJ-33833 HTML-Formbrief: Bestellungs-Anhänge im KV-Kontext\n\n________________________________________________________\nExecuted in   22.68 millis    fish           external\n   usr time    7.71 millis  761.00 micros    6.95 millis\n   sys time   10.47 millis  194.00 micros   10.28 millis\n--8<---------------cut here---------------end--------------->8---\n\nSo you see, it's almost a factor of 1000 difference!  How can that be?\n\nThe split between master and the SHD_ECORO_3_X_X series of branches has\nhappened almost 2 years ago and master is way ahead of those.\n\n--8<---------------cut here---------------start------------->8---\n❯ git log --oneline master...origin/SHD_ECORO_3_9_7 | wc -l\n5013\n--8<---------------cut here---------------end--------------->8---\n\nBut there are around 9 merges from the last release branch into master\ndaily.\n\n--8<---------------cut here---------------start------------->8---\n❯ git log --merges --oneline --since 6months | wc -l\n1611\n--8<---------------cut here---------------end--------------->8---\n\nFrom my memory, the issue hasn't popped up out of sudden but has gotten\nworse slowly over time.  I have the impression that the worsening\nincreased pace over the last few month which might be the result of our\nworkflow.  Before, I've been the merge guy doing two \"merge waves\" from\nthe last supported release branch upwards into master once or twice a\nday (usually release-branch -> next-release-branch -> master).  Since\nabout 3 month, we've switched to a workflow where every developer does\nmerge upwards herself just after committing/pushing to some lesser\nbranch than master simply because branches have diverged so much that\nyou'd need to be an expert in everything in order to be able to resolve\nconflicts sensibly.\n\nI should mention that I haven't seen this issue with any other repo I\nhave.  But that's also the biggest one I use.  The Emacs repository I\nalso work on is comparable in the number of commits but with much less\nmerges.\n\nAt last, here's the git bugreport sysinfo section on that machine and\nrepository.\n\n--8<---------------cut here---------------start------------->8---\n[System Info]\ngit version:\ngit version 2.36.1\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 5.18.1-zen1-1-zen #1 ZEN SMP PREEMPT_DYNAMIC Mon, 30 May 2022 17:53:16 +0000 x86_64\ncompiler info: gnuc: 11.2\nlibc info: glibc: 2.35\n$SHELL (typically, interactive shell): /usr/bin/fish\n\n[Enabled Hooks]\n--8<---------------cut here---------------end--------------->8---\n\nSOLUTION\n========\n\nWhile writing this long mail, I've figured out that the performance\npenalty is caused by my setting of diff.renameLimit = 10000.  If I\ncomment that option in my ~/.gitconfig, the above command finishes in\n150 millis instead of 13 seconds:\n\n--8<---------------cut here---------------start------------->8---\n❯ time git show --no-patch --format=\"%h %s\" \"master^{commit}\" --\n6192a0cfdc6 Merge remote-tracking branch 'origin/SHD_ECORO_3_9_7'\n\n________________________________________________________\nExecuted in  147.99 millis    fish           external\n   usr time  114.52 millis  713.00 micros  113.81 millis\n   sys time   34.78 millis  193.00 micros   34.59 millis\n--8<---------------cut here---------------end--------------->8---\n\nBut there's still the question why diff.renameLimit has an influence\nhere when --no-patch is provided so no diff should be generated.\n\nBye,\nTassilo\n\n[1] https://magit.vc/\n"},{"id":"456692","messageId":"CAPMMpohzqKo-+q-tOcXymmzGxuOY-mf2NPRviHURm8-+3MPjZg@mail.gmail.com","threadId":"57954","inReplyTo":"87h750q1b9.fsf@gnu.org","subject":"Re: [BUG?] Major performance issue with some commands on our repo's master branch","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-06-04T20:20:24Z","receivedAt":"2022-06-04T20:20:40Z","isPatch":false,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"(resending as text-only after having stupidly replied from my mobile)\n\nI can add a couple things that may or may not be related here. I work\nwith a large proprietary repo, like you, and it is also not absurdly\nlarge. I maintain some custom tooling for a large scale perforce\ninterop process.\n\nI used to use \"git show\" (without patch) in this custom tooling to get\ncommit metadata, because it has the advantage that you can specify an\narbitrary list of commits in one call, saving some process overheads\nin Windows especially.\n\nI stopped using \"git show\" when user reports of slowness eventually\nrevealed two things:\n\n1. Large commits (eg merges to feature branches from the fast-moving\nmain trunk) were taking a surprisingly long time, despite the\nno-patch, which made me think it was doing the patch work anyway, and\njust discarding it at the end.\n\n2. Merge commits from long-outdated feature branches, even though the\nfinal patch displayed by \"git show\" is small, also take a long time.\nIt seems as though whatever patch-related work \"git show\" does (and\ngiven your observations I guess it might well be rename-detection), it\ndoes it with respect to *both parents* in the case of a merge request,\neven though the patch it shows is only changes wrt the first parent.\n\nAll this to say: I haven't understood your branch setup, but I'm\nguessing that you're regularly integrating work from \"far-behind\"\nbranches, and most or all of your commits on master are therefore\nmerges with large diffs wrt the second parent, and those large diffs\nwrt the second parent are what's \"getting worse\".\n\nI haven't attempted to debug this, and personally have little\nincentive to do, as switching to \"git log\" and accepting the process\noverheads solved *my* problem.\n\nIf I get the chance to, I will obviously report back here.\n\nThanks,\nTao\n\nOn Sat, Jun 4, 2022 at 10:29 AM Tassilo Horn <tsdh@gnu.org> wrote:\n>\n> Hi all,\n>\n> [spoiler alert: I've figured out the config option causing the problem\n> while writing this long mail, so you might jump straight to the SOLUTION\n> section at the bottom of this mail.]\n>\n> at my day job, I work on a git repo (sadly non-public, proprietary) with\n> these stats:\n>\n> - master has about 150000 commits, the last release branch I've also benchmarked above has 144000 commits\n> - the history dates back to 2001\n> - .git/ is about 1.8 GB\n>\n> So it's quite big but not unusually big when compared to linux or other\n> free software projects.\n>\n> The typical git commands I use (status, fetch, pull, commit, push,\n> rebase, ...) are all quick.  However, I use the git porcelain Magit [1]\n> which invokes several plumbing commands in order to present to the user\n> an always up-to-date extended status buffer of the currently checked out\n> branch showing the current branch.  Some of those plumbing commands are\n> extremely slow for no obvious reasons.  The most outstanding command I\n> could pinpoint is this:\n>\n> --8<---------------cut here---------------start------------->8---\n> ❯ time git show --no-patch --format=\"%h %s\" \"master^{commit}\" --\n> 6192a0cfdc6 Merge remote-tracking branch 'origin/SHD_ECORO_3_9_7'\n>\n> ________________________________________________________\n> Executed in   13.21 secs    fish           external\n>    usr time   12.99 secs  462.00 micros   12.99 secs\n>    sys time    0.17 secs  119.00 micros    0.17 secs\n> --8<---------------cut here---------------end--------------->8---\n>\n> The interesting thing is that I have this problem only with the master\n> branch.  When I run it for the last release branch, I get these times:\n>\n> --8<---------------cut here---------------start------------->8---\n> ❯ time git show --no-patch --format=\"%h %s\" \"SHD_ECORO_3_9_7^{commit}\" --\n> 994334fc9fb ECOJ-33833 HTML-Formbrief: Bestellungs-Anhänge im KV-Kontext\n>\n> ________________________________________________________\n> Executed in   22.68 millis    fish           external\n>    usr time    7.71 millis  761.00 micros    6.95 millis\n>    sys time   10.47 millis  194.00 micros   10.28 millis\n> --8<---------------cut here---------------end--------------->8---\n>\n> So you see, it's almost a factor of 1000 difference!  How can that be?\n>\n> The split between master and the SHD_ECORO_3_X_X series of branches has\n> happened almost 2 years ago and master is way ahead of those.\n>\n> --8<---------------cut here---------------start------------->8---\n> ❯ git log --oneline master...origin/SHD_ECORO_3_9_7 | wc -l\n> 5013\n> --8<---------------cut here---------------end--------------->8---\n>\n> But there are around 9 merges from the last release branch into master\n> daily.\n>\n> --8<---------------cut here---------------start------------->8---\n> ❯ git log --merges --oneline --since 6months | wc -l\n> 1611\n> --8<---------------cut here---------------end--------------->8---\n>\n> From my memory, the issue hasn't popped up out of sudden but has gotten\n> worse slowly over time.  I have the impression that the worsening\n> increased pace over the last few month which might be the result of our\n> workflow.  Before, I've been the merge guy doing two \"merge waves\" from\n> the last supported release branch upwards into master once or twice a\n> day (usually release-branch -> next-release-branch -> master).  Since\n> about 3 month, we've switched to a workflow where every developer does\n> merge upwards herself just after committing/pushing to some lesser\n> branch than master simply because branches have diverged so much that\n> you'd need to be an expert in everything in order to be able to resolve\n> conflicts sensibly.\n>\n> I should mention that I haven't seen this issue with any other repo I\n> have.  But that's also the biggest one I use.  The Emacs repository I\n> also work on is comparable in the number of commits but with much less\n> merges.\n>\n> At last, here's the git bugreport sysinfo section on that machine and\n> repository.\n>\n> --8<---------------cut here---------------start------------->8---\n> [System Info]\n> git version:\n> git version 2.36.1\n> cpu: x86_64\n> no commit associated with this build\n> sizeof-long: 8\n> sizeof-size_t: 8\n> shell-path: /bin/sh\n> uname: Linux 5.18.1-zen1-1-zen #1 ZEN SMP PREEMPT_DYNAMIC Mon, 30 May 2022 17:53:16 +0000 x86_64\n> compiler info: gnuc: 11.2\n> libc info: glibc: 2.35\n> $SHELL (typically, interactive shell): /usr/bin/fish\n>\n> [Enabled Hooks]\n> --8<---------------cut here---------------end--------------->8---\n>\n> SOLUTION\n> ========\n>\n> While writing this long mail, I've figured out that the performance\n> penalty is caused by my setting of diff.renameLimit = 10000.  If I\n> comment that option in my ~/.gitconfig, the above command finishes in\n> 150 millis instead of 13 seconds:\n>\n> --8<---------------cut here---------------start------------->8---\n> ❯ time git show --no-patch --format=\"%h %s\" \"master^{commit}\" --\n> 6192a0cfdc6 Merge remote-tracking branch 'origin/SHD_ECORO_3_9_7'\n>\n> ________________________________________________________\n> Executed in  147.99 millis    fish           external\n>    usr time  114.52 millis  713.00 micros  113.81 millis\n>    sys time   34.78 millis  193.00 micros   34.59 millis\n> --8<---------------cut here---------------end--------------->8---\n>\n> But there's still the question why diff.renameLimit has an influence\n> here when --no-patch is provided so no diff should be generated.\n>\n> Bye,\n> Tassilo\n>\n> [1] https://magit.vc/\n"},{"id":"456697","messageId":"87y1yb2xc8.fsf@gnu.org","threadId":"57954","inReplyTo":"CAPMMpohzqKo-+q-tOcXymmzGxuOY-mf2NPRviHURm8-+3MPjZg@mail.gmail.com","subject":"Re: [BUG?] Major performance issue with some commands on our repo's master branch","fromName":"Tassilo Horn","fromEmail":"tsdh@gnu.org","sentAt":"2022-06-05T10:46:15Z","receivedAt":"2022-06-05T10:56:02Z","isPatch":false,"sender":{"key":"tsdh@gnu.org","avatar":"https://avatars.githubusercontent.com/u/103854?v=4"},"body":"Tao Klerks <tao@klerks.biz> writes:\n\nHi Tao,\n\nthanks for your response.\n\n> All this to say: I haven't understood your branch setup, but I'm\n> guessing that you're regularly integrating work from \"far-behind\"\n> branches, and most or all of your commits on master are therefore\n> merges with large diffs wrt the second parent, and those large diffs\n> wrt the second parent are what's \"getting worse\".\n\nThat's exactly correct.\n\n> I haven't attempted to debug this, and personally have little\n> incentive to do, as switching to \"git log\" and accepting the process\n> overheads solved *my* problem.\n\nAnd I'm happy to report it solves *my* problem as well.  There's a PR\nfor the Magit git porcelain replacing \"git show\" with an equivalent \"git\nlog\" incarnation which makes the 30seconds \"refresh status buffer\"\noperation instant.\n\n  https://github.com/magit/magit/issues/4702\n  https://github.com/magit/magit/compare/km/show-to-log\n\nStill maybe someone might want to have a look at the \"git show\" issue to\ndouble-check if the performance burden in this specific case (no diff\nshould be generated) is warranted.  But at least I can work again with\nno coffee-break long pauses, so I'm all satisfied. :-)\n\nThanks a lot for your insights.\n\nBye,\nTassilo\n"},{"id":"456703","messageId":"CAPMMpog-7eDOrgSU9GjV4G9rk5RkL-PJhaUAO3_0p2YxfRf=LA@mail.gmail.com","threadId":"57954","inReplyTo":"87y1yb2xc8.fsf@gnu.org","subject":"Re: [BUG?] Major performance issue with some commands on our repo's master branch","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-06-06T05:18:55Z","receivedAt":"2022-06-06T05:29:26Z","isPatch":false,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Sun, Jun 5, 2022 at 12:55 PM Tassilo Horn <tsdh@gnu.org> wrote:\n>\n> Tao Klerks <tao@klerks.biz> writes:\n>\n> > I haven't attempted to debug this, and personally have little\n> > incentive to do, as switching to \"git log\" and accepting the process\n> > overheads solved *my* problem.\n>\n> And I'm happy to report it solves *my* problem as well.  There's a PR\n> for the Magit git porcelain replacing \"git show\" with an equivalent \"git\n> log\" incarnation which makes the 30seconds \"refresh status buffer\"\n> operation instant.\n>\n>   https://github.com/magit/magit/issues/4702\n>   https://github.com/magit/magit/compare/km/show-to-log\n>\n> Still maybe someone might want to have a look at the \"git show\" issue to\n> double-check if the performance burden in this specific case (no diff\n> should be generated) is warranted.\n\nI spent a little time with this yesterday, and can confirm:\n* My issue seems to be the same as yours, \"export GIT_TRACE2_PERF=1\"\nshows all the time being spent in rename detection\n* \"git show\" is a slightly different entry point into the \"git log\"\ncode (log.c, cmd_show())\n* options to the \"git log\" functionality are largely collected in a\n\"rev_info\" object (defined in revision.h)\n* one option is the \"-c / --diff-merges=combined\" option of \"git log\"\n(setting rev_info.diff, rev_info.combine_merges and\nrev_info.dense_combined_merges)\n* another option is \"-s / --no-patch\" option of \"git log\" (setting\nrev_info.diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT)\n* the \"--patch\" and \"--no-patch\" options seem to be (and are\ndocumented as) opposites, but they are not implemented as such; one\ncalls for the work to be done, and the other only hides the output.\n* the \"diffs\" options are set automatically/implicitly for \"git show\",\nbefore argument parsing\n* we can simulate the \"git show\" performance issue in \"--git log -1\"\nby setting \"--diff-merges=combined\" *and* \"--no-patch\" explicitly\n* this performance issue does *not* present in a \"git log\" call with\nonly the regular \"--patch\" argument, however; this basic \"show\npatches\" instruction defaults to \"--diff-merges=off\", which means the\nrename detection work does not need to happen. There might still be\nslight diff-related overheads, but they are undetectable in my\ntesting.\n\nTherefore, I have confirmed it is possible to get \"git show\" to behave\nthe way we would expect for \"--no-patch\", by *also* specifying\n\"--diff-merges=off\".\n\nThere are at least two possible approaches / directions to improving\nthese issues generically in the \"git show\" implementation, I think:\n\n1. Add some \"git show\"-specific code, saying something along the lines\nof \"if --no-patch is specified, then also imply \"--diff-merges=off\".\nThis feels like the safer option / less likely to have side-effects.\n\n2. Add some post-processing to the generic \"git log & git show\"\noptions parsing, to generically propagate \"--no-patch\" into other\nproperties like those set by \"--diff-merges=combined\"\n\nI don't feel confident enough with the code here to try for the second\napproach, but the first looks like something I should be able to\npropose a patch for - and in the meantime I know how to get the\n\"single-process, many arbitrary commits\" performance benefit of \"git\nshow\" again. Thanks for sparking the exploration down this little\nrabbit-hole!\n"},{"id":"456910","messageId":"YqEyh5opAaJxph2+@coredump.intra.peff.net","threadId":"57954","inReplyTo":"87y1yb2xc8.fsf@gnu.org","subject":"Re: [BUG?] Major performance issue with some commands on our repo's master branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-06-08T23:36:39Z","receivedAt":"2022-06-08T23:36:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 05, 2022 at 12:46:15PM +0200, Tassilo Horn wrote:\n\n> Still maybe someone might want to have a look at the \"git show\" issue to\n> double-check if the performance burden in this specific case (no diff\n> should be generated) is warranted.  But at least I can work again with\n> no coffee-break long pauses, so I'm all satisfied. :-)\n\nI suspect the issue may be quite subtle. Even you asked for\n\"--no-patch\", the underlying diff may still be used for other things.\nFor example, simplifying away TREESAME commits. I.e., ones which did not\nchange anything from their parents after applying path restrictions,\ndiff-filters, etc. There may be other cases, too (e.g., --follow).\n\nI think the code could be written to realize that none of those features\nare in use, and disable the diff entirely in favor of checking whether\nthe two trees has the same object id. That would yield _mostly_ the same\nbehavior, though there are probably corner cases (e.g., a tree with an\nodd mode entry, say, may get parsed so as to produce an empty diff, even\nthough it's not byte for byte identical). That may be an acceptable\ntradeoff. But I think the code would be a bit brittle (it needs to know\nabout all the cases where a diff might matter, and we may add more\nlater).\n\nIn general, I think Git assumes that tree-level diffs aren't too painful\nto produce. \"git log\" will do them, too, but just doesn't tickle your\nparticular case because it doesn't look at merges. So probably setting\ndiff.renamelimit correctly is not that bad a solution.\n\n-Peff\n"},{"id":"456912","messageId":"87sfoe7hio.fsf@kyleam.com","threadId":"57954","inReplyTo":"YqEyh5opAaJxph2+@coredump.intra.peff.net","subject":"Re: [BUG?] Major performance issue with some commands on our repo's master branch","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2022-06-09T01:27:43Z","receivedAt":"2022-06-09T01:27:54Z","isPatch":false,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"Jeff King writes:\n\n> I suspect the issue may be quite subtle. Even you asked for\n> \"--no-patch\", the underlying diff may still be used for other things.\n> For example, simplifying away TREESAME commits. I.e., ones which did not\n> change anything from their parents after applying path restrictions,\n> diff-filters, etc. There may be other cases, too (e.g., --follow).\n>\n> I think the code could be written to realize that none of those features\n> are in use, and disable the diff entirely in favor of checking whether\n> the two trees has the same object id. That would yield _mostly_ the same\n> behavior, though there are probably corner cases (e.g., a tree with an\n> odd mode entry, say, may get parsed so as to produce an empty diff, even\n> though it's not byte for byte identical). That may be an acceptable\n> tradeoff. But I think the code would be a bit brittle (it needs to know\n> about all the cases where a diff might matter, and we may add more\n> later).\n\nDo you think it'd be safe to make --no-patch imply --diff-merges=off, as\nTao suggested elsewhere in this thread?\n\n  https://lore.kernel.org/git/CAPMMpog-7eDOrgSU9GjV4G9rk5RkL-PJhaUAO3_0p2YxfRf=LA@mail.gmail.com\n\nIf so, it seems like that'd be a good way to get speedups for some merge\ncommits.  For example, here are hyperfine timings for the current tip of\ngit.git's master branch:\n\n  Benchmark #1: git show --no-patch --format=%h 1e59178e3f\n    Time (mean ± σ):      47.8 ms ±   1.5 ms    [User: 43.2 ms, System: 4.6 ms]\n    Range (min … max):    46.8 ms …  54.4 ms    59 runs\n   \n    Warning: Statistical outliers were detected. Consider re-running\n    this benchmark on a quiet PC without any interferences from other\n    programs. It might help to use the '--warmup' or '--prepare'\n    options.\n   \n  Benchmark #2: git show --no-patch --diff-merges=off --format=%h 1e59178e3f\n    Time (mean ± σ):       3.2 ms ±   0.2 ms    [User: 2.5 ms, System: 0.8 ms]\n    Range (min … max):     2.9 ms …   6.8 ms    688 runs\n   \n    Warning: Command took less than 5 ms to complete. Results might be\n    inaccurate.\n    \n    Warning: Statistical outliers were detected. Consider [...]\n    options.\n   \n  Benchmark #3: git log --no-walk --format=%h 1e59178e3f\n    Time (mean ± σ):       3.2 ms ±   0.1 ms    [User: 2.4 ms, System: 0.8 ms]\n    Range (min … max):     2.9 ms …   4.2 ms    697 runs\n   \n    Warning: Command took less than 5 ms to complete. Results might [...]\n    \n    Warning: Statistical outliers were detected. Consider [...]\n    \n   \n  Summary\n    'git log --no-walk --format=%h 1e59178e3f' ran\n      1.01 ± 0.08 times faster than 'git show --no-patch --diff-merges=off --format=%h 1e59178e3f'\n     14.98 ± 0.79 times faster than 'git show --no-patch --format=%h 1e59178e3f'\n"},{"id":"456917","messageId":"87mtembcjl.fsf@gnu.org","threadId":"57954","inReplyTo":"YqEyh5opAaJxph2+@coredump.intra.peff.net","subject":"Re: [BUG?] Major performance issue with some commands on our repo's master branch","fromName":"Tassilo Horn","fromEmail":"tsdh@gnu.org","sentAt":"2022-06-09T05:51:36Z","receivedAt":"2022-06-09T06:01:47Z","isPatch":false,"sender":{"key":"tsdh@gnu.org","avatar":"https://avatars.githubusercontent.com/u/103854?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\nHi Jeff,\n\n>> Still maybe someone might want to have a look at the \"git show\" issue\n>> to double-check if the performance burden in this specific case (no\n>> diff should be generated) is warranted.  But at least I can work\n>> again with no coffee-break long pauses, so I'm all satisfied. :-)\n>\n> I suspect the issue may be quite subtle. Even you asked for\n> \"--no-patch\", the underlying diff may still be used for other things.\n> For example, simplifying away TREESAME commits. I.e., ones which did\n> not change anything from their parents after applying path\n> restrictions, diff-filters, etc. There may be other cases, too (e.g.,\n> --follow).\n\nI see.  In the end, my issue was solved by my git porcelain switching to\na \"git log\" incarnation instead of \"git show\".  When I \"git show\"\nmanually, it's no big deal if it takes some time for merge commits.\n\n> [...]\n>\n> So probably setting diff.renamelimit correctly is not that bad a\n> solution.\n\nDoes your statement imply diff.renameLimit = 10000 is an incorrect\nsetting?  The thing is that I mostly work with java codebases where\nevery file rename implies a change in file contents, too.  A large\nrenameLimit seems to help in correctly detecting renames/copies although\nI don't have empirical data but only gut feeling.\n\nBye,\nTassilo\n"},{"id":"456943","messageId":"YqILyX97zKg5ViUS@coredump.intra.peff.net","threadId":"57954","inReplyTo":"87sfoe7hio.fsf@kyleam.com","subject":"Re: [BUG?] Major performance issue with some commands on our repo's master branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-06-09T15:03:37Z","receivedAt":"2022-06-09T15:03:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 08, 2022 at 09:27:43PM -0400, Kyle Meyer wrote:\n\n> > I think the code could be written to realize that none of those features\n> > are in use, and disable the diff entirely in favor of checking whether\n> > the two trees has the same object id. That would yield _mostly_ the same\n> > behavior, though there are probably corner cases (e.g., a tree with an\n> > odd mode entry, say, may get parsed so as to produce an empty diff, even\n> > though it's not byte for byte identical). That may be an acceptable\n> > tradeoff. But I think the code would be a bit brittle (it needs to know\n> > about all the cases where a diff might matter, and we may add more\n> > later).\n> \n> Do you think it'd be safe to make --no-patch imply --diff-merges=off, as\n> Tao suggested elsewhere in this thread?\n> \n>   https://lore.kernel.org/git/CAPMMpog-7eDOrgSU9GjV4G9rk5RkL-PJhaUAO3_0p2YxfRf=LA@mail.gmail.com\n\nI'm not sure. If I do:\n\ndiff --git a/diff-merges.c b/diff-merges.c\nindex 7f64156b8b..f05a585dfb 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -164,6 +164,9 @@ void diff_merges_set_dense_combined_if_unset(struct rev_info *revs)\n \n void diff_merges_setup_revs(struct rev_info *revs)\n {\n+\tif (revs->diffopt.output_format == DIFF_FORMAT_NO_OUTPUT &&\n+\t    !revs->explicit_diff_merges)\n+\t\tdiff_merges_suppress(revs);\n \tif (revs->combine_merges == 0)\n \t\trevs->dense_combined_merges = 0;\n \tif (revs->separate_merges == 0)\n\nthen the test suite passes, but that may just be because we are not\ninvoking the right corner case. It does change the output with something\nlike:\n\n  git show --diff-filter=D -s a6434bc6f7a1\n\nWithout the patch above, it always shows the commit. With it, it shows\nnothing. That's a bit far-fetched, but it is a regression, and I'm also\nnot sure if it's just the tip of the iceberg.\n\nIt also doesn't solve problem completely. Regular commits can have\nexpensive diffs, too.\n\nI think you'd do better to have a mode specific to git-show that skips\nthe diff if we're not showing it, but makes sure to always show the\ncommit anyway. Perhaps something like the hunk above, but put into\ncmd_show(), and then setting revs->always_show_header. But it would\nrequire somebody verifying that this does the right thing in all cases.\n\n-Peff\n"},{"id":"456944","messageId":"YqIMTYR2wM8iZCUN@coredump.intra.peff.net","threadId":"57954","inReplyTo":"87mtembcjl.fsf@gnu.org","subject":"Re: [BUG?] Major performance issue with some commands on our repo's master branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-06-09T15:05:49Z","receivedAt":"2022-06-09T15:05:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 09, 2022 at 07:51:36AM +0200, Tassilo Horn wrote:\n\n> > So probably setting diff.renamelimit correctly is not that bad a\n> > solution.\n> \n> Does your statement imply diff.renameLimit = 10000 is an incorrect\n> setting?  The thing is that I mostly work with java codebases where\n> every file rename implies a change in file contents, too.  A large\n> renameLimit seems to help in correctly detecting renames/copies although\n> I don't have empirical data but only gut feeling.\n\nWell, for some definition of incorrect. :) You are telling Git to spend\nextra time computing renames, and then you were annoyed when it spent a\nlong time computing renames. So in that sense it was not what you\nwanted.\n\nIt may be that you want different limits in different contexts, and the\ncurrent config is not sufficient to express that.\n\n-Peff\n"},{"id":"456956","messageId":"xmqqedzxlmpt.fsf@gitster.g","threadId":"57954","inReplyTo":"YqILyX97zKg5ViUS@coredump.intra.peff.net","subject":"Re: [BUG?] Major performance issue with some commands on our repo's master branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-09T18:23:58Z","receivedAt":"2022-06-09T18:24:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>   git show --diff-filter=D -s a6434bc6f7a1\n>\n> Without the patch above, it always shows the commit. With it, it shows\n> nothing. That's a bit far-fetched, but it is a regression, and I'm also\n> not sure if it's just the tip of the iceberg.\n\nHere \"-s\" is merely \"do not give patch output like we do by\ndefault\", so the behaviour is quite understandable and is not a\nregression we would want to see happen.  -S/-G are also likely to be\naffected, not just the --diff-filter.\n\n> It also doesn't solve problem completely. Regular commits can have\n> expensive diffs, too.\n\nThat's a good point.\n\n> I think you'd do better to have a mode specific to git-show that skips\n> the diff if we're not showing it, but makes sure to always show the\n> commit anyway.\n\nMeaning an explicit option \"git show --log-only\"?  We'd need to\ncareful to make it either (1) be incompatible with certain features\nof \"git show\" (like giving a pathspec) and error out, or (2) ignore\nthese features of \"git show\" silently and document that.  But it\nwould work as a new option.\n\nThanks.\n"},{"id":"456960","messageId":"YqI/TcZyXomxtXtN@coredump.intra.peff.net","threadId":"57954","inReplyTo":"xmqqedzxlmpt.fsf@gitster.g","subject":"Re: [BUG?] Major performance issue with some commands on our repo's master branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-06-09T18:43:25Z","receivedAt":"2022-06-09T18:43:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 09, 2022 at 11:23:58AM -0700, Junio C Hamano wrote:\n\n> > I think you'd do better to have a mode specific to git-show that skips\n> > the diff if we're not showing it, but makes sure to always show the\n> > commit anyway.\n> \n> Meaning an explicit option \"git show --log-only\"?  We'd need to\n> careful to make it either (1) be incompatible with certain features\n> of \"git show\" (like giving a pathspec) and error out, or (2) ignore\n> these features of \"git show\" silently and document that.  But it\n> would work as a new option.\n\nCertainly a new option would mean we weren't regressing any existing\nbehavior. I do think it would be a hard option to explain, though.\n\nBut what I wondered is whether \"show\" in particular, because it would\nnever want to skip showing a commit, could get away with avoiding the\ndiff automatically. I.e., currently \"git show -Sfoo HEAD\" will always\nshow HEAD, even if \"-S\" does not match anything. So if we are not\nshowing any diff output, there is no need to compute the diff in that\ncase. That is unlike \"git log\", which would omit commits that didn't\nmatch.\n\nAnd really it is not \"git show\" that is special there, but the\nalways_show_header flag it sets. So something like this might work:\n\ndiff --git a/log-tree.c b/log-tree.c\nindex d0ac0a6327..ed57386938 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -1024,6 +1024,10 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log\n \tif (!all_need_diff && !opt->merges_need_diff)\n \t\treturn 0;\n \n+\tif (opt->diffopt.output_format == DIFF_FORMAT_NO_OUTPUT &&\n+\t    opt->always_show_header)\n+\t\treturn 0;\n+\n \tparse_commit_or_die(commit);\n \toid = get_commit_tree_oid(commit);\n \n\nIt produces the same output in the cases I tried. And running with\nGIT_TRACE2_PERF shows that it doesn't diff and rename code.\n\nI'm not overly confident that it isn't violating some other subtle\nassumption / corner case that I haven't thought of, though. :)\n\n-Peff\n"},{"id":"456964","messageId":"xmqqtu8tiotj.fsf@gitster.g","threadId":"57954","inReplyTo":"YqI/TcZyXomxtXtN@coredump.intra.peff.net","subject":"Re: [BUG?] Major performance issue with some commands on our repo's master branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-09T20:06:48Z","receivedAt":"2022-06-09T20:06:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> But what I wondered is whether \"show\" in particular, because it would\n> never want to skip showing a commit, could get away with avoiding the\n> diff automatically.\n\nAhh, that is a clever thought.  At least unless we automatically\nturn ourselves into \"git log\" by giving an range, we are naming\nindividual object we want to see, so why not show them?\n\nBut I wonder if \"git show A -- P\" should or should not show the\ncommit if A does not touch the path P.  Right now we apply the same\nhistory simplification so \"git show master -- t/\" gives nothing to\nme, which is probably one sensible thing to do.  It is debatable why\nsomebody who wants to see 'master' wants to hide it when it does not\ntouch the paths that match the pathspec given, but it can also be\ndebated why somebody would give a pathspec if commits are to be\nhidden when they do not touch paths that match it, so...\n\n> I.e., currently \"git show -Sfoo HEAD\" will always\n> show HEAD, even if \"-S\" does not match anything. So if we are not\n> showing any diff output, there is no need to compute the diff in that\n> case. That is unlike \"git log\", which would omit commits that didn't\n> match.\n\nOK, you came up with an example that behaves differently.\n\n> And really it is not \"git show\" that is special there, but the\n> always_show_header flag it sets. So something like this might work:\n\nA tempting thought, indeed.\n\n> diff --git a/log-tree.c b/log-tree.c\n> index d0ac0a6327..ed57386938 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -1024,6 +1024,10 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log\n>  \tif (!all_need_diff && !opt->merges_need_diff)\n>  \t\treturn 0;\n>  \n> +\tif (opt->diffopt.output_format == DIFF_FORMAT_NO_OUTPUT &&\n> +\t    opt->always_show_header)\n> +\t\treturn 0;\n> +\n>  \tparse_commit_or_die(commit);\n>  \toid = get_commit_tree_oid(commit);\n>  \n>\n> It produces the same output in the cases I tried. And running with\n> GIT_TRACE2_PERF shows that it doesn't diff and rename code.\n>\n> I'm not overly confident that it isn't violating some other subtle\n> assumption / corner case that I haven't thought of, though. :)\n>\n> -Peff\n"}]}