{"thread":{"id":"51812","subject":"[PATCH 0/2] a few commit-graph improvements","startedAt":"2019-09-07T04:58:53Z","lastAt":"2019-09-27T18:54:32Z","messageCount":8,"participants":["Jeff King","SZEDER Gábor","Taylor Blau","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"381996","messageId":"20190907045848.GA24515@sigill.intra.peff.net","threadId":"51812","inReplyTo":null,"subject":"[PATCH 0/2] a few commit-graph improvements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-07T04:58:48Z","receivedAt":"2019-09-07T04:58:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We've been playing with commit graphs at GitHub and found a few bits of\nlow-hanging fruit (one liners -- it doesn't get any lower than that).\n\nThe first one is actually a resurrection of a patch from March:\n\n https://public-inbox.org/git/20190322102817.19708-1-szeder.dev@gmail.com/\n\nwhere the progress bar sometimes prints nonsense. There's some\ndiscussion in that thread about how we could sometimes show a real\npercentage instead of a counting-up progress meter. But given the number\nof corner cases discussed, and the fact that nothing has happened for 6\nmonths, I think we should first make sure we're always doing the\n_correct_ thing, and then people can build a nicer meter on top if they\nwant to.\n\nThe second is a fix for a small memory \"leak\", but it makes a big\ndifference.\n\n [1/2]: commit-graph: don't show progress percentages while expanding reachable commits\n [2/2]: commit-graph: turn off save_commit_buffer\n\n builtin/commit-graph.c | 2 ++\n commit-graph.c | 2 +-\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\n-Peff\n"},{"id":"381997","messageId":"20190907050132.GA23904@sigill.intra.peff.net","threadId":"51812","inReplyTo":"20190907045848.GA24515@sigill.intra.peff.net","subject":"[PATCH 1/2] commit-graph: don't show progress percentages while expanding reachable commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-07T05:01:33Z","receivedAt":"2019-09-07T05:04:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: SZEDER Gábor \n\nCommit 49bbc57a57 (commit-graph write: emit a percentage for all\nprogress, 2019-01-19) was a bit overeager when it added progress\npercentages to the \"Expanding reachable commits in commit graph\" phase\nas well, because most of the time the number of commits that phase has\nto iterate over is not known in advance and grows significantly, and,\nconsequently, we end up with nonsensical numbers:\n\n $ git commit-graph write --reachable\n Expanding reachable commits in commit graph: 138606% (824706/595), done.\n [...]\n\n $ git rev-parse v5.0 | git commit-graph write --stdin-commits\n Expanding reachable commits in commit graph: 81264400% (812644/1), done.\n [...]\n\nEven worse, because the percentage grows so quickly, the progress code\noutputs much more often than it should (because it ticks every second,\nor every 1%), slowing the whole process down. My time for \"git\ncommit-graph write --reachable\" on linux.git went from 13.463s to\n12.521s with this patch, ~7% savings.\n\nTherefore, don't show progress percentages in the \"Expanding reachable\ncommits in commit graph\" phase.\n\nNote that the current code does sometimes do the right thing, if we\npicked up all commits initially (e.g., omitting \"--reachable\" in a\nfully-packed repository would get the correct count without any parent\ntraversal). So it may be possible to come up with a way to tell when we\ncould use a percentage here. But in the meantime, let's make sure we\nrobustly avoid printing nonsense.\n\nSigned-off-by: SZEDER Gábor \nSigned-off-by: Jeff King \n---\nCompared to the original from:\n\n https://public-inbox.org/git/20190322102817.19708-1-szeder.dev@gmail.com/\n\nI rebased it to handle code movement, added in the timing data, and\ntried to summarize the discussion from the thread.\n\n commit-graph.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex f2888c203b..d6a5c8cf1c 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1050,7 +1050,7 @@ static void close_reachable(struct write_commit_graph_context *ctx)\n \tif (ctx->report_progress)\n \t\tctx->progress = start_delayed_progress(\n \t\t\t\t\t_(\"Expanding reachable commits in commit graph\"),\n-\t\t\t\t\tctx->oids.nr);\n+\t\t\t\t\t0);\n \tfor (i = 0; i < ctx->oids.nr; i++) {\n \t\tdisplay_progress(ctx->progress, i + 1);\n \t\tcommit = lookup_commit(ctx->r, &ctx->oids.list[i]);\n-- \n2.23.0.474.gb1abd76f7a\n"},{"id":"381998","messageId":"20190907050439.GB23904@sigill.intra.peff.net","threadId":"51812","inReplyTo":"20190907045848.GA24515@sigill.intra.peff.net","subject":"[PATCH 2/2] commit-graph: turn off save_commit_buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-07T05:04:40Z","receivedAt":"2019-09-07T05:04:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The commit-graph tool may read a lot of commits, but it only cares about\nparsing their metadata (parents, trees, etc) and doesn't ever show the\nmessages to the user. And so it should not need save_commit_buffer,\nwhich is meant for holding onto the object data of parsed commits so\nthat we can show them later. In fact, it's quite harmful to do so.\nAccording to massif, the max heap of \"git commit-graph write\n--reachable\" in linux.git before/after this patch (removing the commit\ngraph file in between) goes from ~1.1GB to ~270MB.\n\nWhich isn't surprising, since the difference is about the sum of the\nuncompressed sizes of all commits in the repository, and this was\nequivalent to leaking them.\n\nThis obviously helps if you're under memory pressure, but even without\nit, things go faster. My before/after times for that command (without\nmassif) went from 12.521s to 11.874s, a speedup of ~5%.\n\nSigned-off-by: Jeff King \n---\nWe didn't actually notice this on linux.git, but rather on a repository\nwith 130 million commits (don't ask). With this patch, I was able to\ngenerate the commit-graph file with a peak heap of ~25GB, which is ~200\nbytes per commit.\n\nI'll bet we could do better with some effort, but obviously this case\nwas just pathological. For most cases this should be cheaper than a\nnormal repack (which probably spends that much memory on each object,\nnot just commits).\n\n builtin/commit-graph.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 57863619b7..052696f1af 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -251,6 +251,8 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)\n \t\t\t builtin_commit_graph_usage,\n \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n \n+\tsave_commit_buffer = 0;\n+\n \tif (argc > 0) {\n \t\tif (!strcmp(argv[0], \"read\"))\n \t\t\treturn graph_read(argc, argv);\n-- \n2.23.0.474.gb1abd76f7a\n"},{"id":"382004","messageId":"20190907103407.GF32087@szeder.dev","threadId":"51812","inReplyTo":"20190907050132.GA23904@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] commit-graph: don't show progress percentages while expanding reachable commits","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-09-07T10:34:07Z","receivedAt":"2019-09-07T10:34:14Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sat, Sep 07, 2019 at 01:01:33AM -0400, Jeff King wrote:\n> From: SZEDER Gábor \n> \n> Commit 49bbc57a57 (commit-graph write: emit a percentage for all\n> progress, 2019-01-19) was a bit overeager when it added progress\n> percentages to the \"Expanding reachable commits in commit graph\" phase\n> as well, because most of the time the number of commits that phase has\n> to iterate over is not known in advance and grows significantly, and,\n> consequently, we end up with nonsensical numbers:\n> \n> $ git commit-graph write --reachable\n> Expanding reachable commits in commit graph: 138606% (824706/595), done.\n> [...]\n> \n> $ git rev-parse v5.0 | git commit-graph write --stdin-commits\n> Expanding reachable commits in commit graph: 81264400% (812644/1), done.\n> [...]\n> \n> Even worse, because the percentage grows so quickly, the progress code\n> outputs much more often than it should (because it ticks every second,\n> or every 1%), slowing the whole process down. My time for \"git\n> commit-graph write --reachable\" on linux.git went from 13.463s to\n> 12.521s with this patch, ~7% savings.\n\nOh, interesting.\n\n> Therefore, don't show progress percentages in the \"Expanding reachable\n> commits in commit graph\" phase.\n> \n> Note that the current code does sometimes do the right thing, if we\n> picked up all commits initially (e.g., omitting \"--reachable\" in a\n> fully-packed repository would get the correct count without any parent\n> traversal). So it may be possible to come up with a way to tell when we\n> could use a percentage here. But in the meantime, let's make sure we\n> robustly avoid printing nonsense.\n> \n> Signed-off-by: SZEDER Gábor \n> Signed-off-by: Jeff King \n> ---\n> Compared to the original from:\n> \n> https://public-inbox.org/git/20190322102817.19708-1-szeder.dev@gmail.com/\n> \n> I rebased it to handle code movement, added in the timing data, and\n> tried to summarize the discussion from the thread.\n\nThanks for resurrecting this patch and for the summary paragraph.\n\n"},{"id":"382024","messageId":"20190907185458.GA32028@syl.local","threadId":"51812","inReplyTo":"20190907103407.GF32087@szeder.dev","subject":"Re: [PATCH 1/2] commit-graph: don't show progress percentages while expanding reachable commits","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-09-07T18:54:58Z","receivedAt":"2019-09-07T18:55:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Sep 07, 2019 at 12:34:07PM +0200, SZEDER Gábor wrote:\n> On Sat, Sep 07, 2019 at 01:01:33AM -0400, Jeff King wrote:\n> > From: SZEDER Gábor \n> >\n> > Commit 49bbc57a57 (commit-graph write: emit a percentage for all\n> > progress, 2019-01-19) was a bit overeager when it added progress\n> > percentages to the \"Expanding reachable commits in commit graph\" phase\n> > as well, because most of the time the number of commits that phase has\n> > to iterate over is not known in advance and grows significantly, and,\n> > consequently, we end up with nonsensical numbers:\n> >\n> > $ git commit-graph write --reachable\n> > Expanding reachable commits in commit graph: 138606% (824706/595), done.\n> > [...]\n> >\n> > $ git rev-parse v5.0 | git commit-graph write --stdin-commits\n> > Expanding reachable commits in commit graph: 81264400% (812644/1), done.\n> > [...]\n> >\n> > Even worse, because the percentage grows so quickly, the progress code\n> > outputs much more often than it should (because it ticks every second,\n> > or every 1%), slowing the whole process down. My time for \"git\n> > commit-graph write --reachable\" on linux.git went from 13.463s to\n> > 12.521s with this patch, ~7% savings.\n>\n> Oh, interesting.\n>\n> > Therefore, don't show progress percentages in the \"Expanding reachable\n> > commits in commit graph\" phase.\n> >\n> > Note that the current code does sometimes do the right thing, if we\n> > picked up all commits initially (e.g., omitting \"--reachable\" in a\n> > fully-packed repository would get the correct count without any parent\n> > traversal). So it may be possible to come up with a way to tell when we\n> > could use a percentage here. But in the meantime, let's make sure we\n> > robustly avoid printing nonsense.\n> >\n> > Signed-off-by: SZEDER Gábor \n> > Signed-off-by: Jeff King \n> > ---\n> > Compared to the original from:\n> >\n> > https://public-inbox.org/git/20190322102817.19708-1-szeder.dev@gmail.com/\n> >\n> > I rebased it to handle code movement, added in the timing data, and\n> > tried to summarize the discussion from the thread.\n>\n> Thanks for resurrecting this patch and for the summary paragraph.\n\nThanks from me, as well. I noticed that we had achieved three billion\npercent progress on the repository that brought this to our attention,\nbut didn't notice that you had already written these patches.\n\nSo, I am glad that they are getting the attribution that they deserve.\nThanks again both.\n\nThanks,\nTaylor\n"},{"id":"382025","messageId":"20190907185636.GB32028@syl.local","threadId":"51812","inReplyTo":"20190907050439.GB23904@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] commit-graph: turn off save_commit_buffer","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-09-07T18:56:36Z","receivedAt":"2019-09-07T18:56:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Sep 07, 2019 at 01:04:40AM -0400, Jeff King wrote:\n> The commit-graph tool may read a lot of commits, but it only cares about\n> parsing their metadata (parents, trees, etc) and doesn't ever show the\n> messages to the user. And so it should not need save_commit_buffer,\n> which is meant for holding onto the object data of parsed commits so\n> that we can show them later. In fact, it's quite harmful to do so.\n> According to massif, the max heap of \"git commit-graph write\n> --reachable\" in linux.git before/after this patch (removing the commit\n> graph file in between) goes from ~1.1GB to ~270MB.\n>\n> Which isn't surprising, since the difference is about the sum of the\n> uncompressed sizes of all commits in the repository, and this was\n> equivalent to leaking them.\n>\n> This obviously helps if you're under memory pressure, but even without\n> it, things go faster. My before/after times for that command (without\n> massif) went from 12.521s to 11.874s, a speedup of ~5%.\n>\n> Signed-off-by: Jeff King \n> ---\n> We didn't actually notice this on linux.git, but rather on a repository\n> with 130 million commits (don't ask). With this patch, I was able to\n> generate the commit-graph file with a peak heap of ~25GB, which is ~200\n> bytes per commit.\n>\n> I'll bet we could do better with some effort, but obviously this case\n> was just pathological. For most cases this should be cheaper than a\n> normal repack (which probably spends that much memory on each object,\n> not just commits).\n>\n> builtin/commit-graph.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index 57863619b7..052696f1af 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -251,6 +251,8 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)\n> \t\t\t builtin_commit_graph_usage,\n> \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n>\n> +\tsave_commit_buffer = 0;\n> +\n\nThis looks exactly right to me. We had discussed a little bit off-list\nabout where you might place this line, but I think that the spot you\npicked is perfect: as late as possible.\n\nThankfully, the option parsing code here doesn't load any commits\n(though even if it did, I don't think that turning on/off\n'save_commit_buffer' would really make much of a difference).\n\nSo, the patch here looks obviously correct, and I don't think it needs a\ntest or anything like that... besides: what is there to test? :).\n\n> \tif (argc > 0) {\n> \t\tif (!strcmp(argv[0], \"read\"))\n> \t\t\treturn graph_read(argc, argv);\n> --\n> 2.23.0.474.gb1abd76f7a\n\nThanks,\nTaylor\n"},{"id":"382042","messageId":"20190908103118.GD15641@sigill.intra.peff.net","threadId":"51812","inReplyTo":"20190907185636.GB32028@syl.local","subject":"Re: [PATCH 2/2] commit-graph: turn off save_commit_buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-08T10:31:19Z","receivedAt":"2019-09-08T10:31:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 07, 2019 at 02:56:36PM -0400, Taylor Blau wrote:\n\n> So, the patch here looks obviously correct, and I don't think it needs a\n> test or anything like that... besides: what is there to test? :).\n\nThere's no functional change, so as long as this has coverage in the\nregular suite (and I think it does), there's no point in adding a new\nfunctional test.\n\nHowever, it may make sense to get some coverage of commit graphs in the\nperf suite, so that we could detect regressions there. I think that\nwould make sense as a separate series, though, not attached to this\nparticular fix. (It would also be cool if the perf suite could record\npeak memory usage, but that would take a fair bit of refactoring).\n\n-Peff\n"},{"id":"383071","messageId":"CAHk-=wjk5c=zKVb_EbB5FWyNGD6d3GW8ayB3=2Boo83uq7JBiA@mail.gmail.com","threadId":"51812","inReplyTo":"20190907050132.GA23904@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] commit-graph: don't show progress percentages while expanding reachable commits","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2019-09-27T18:54:11Z","receivedAt":"2019-09-27T18:54:32Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"I was going to make a bug-report about this funny behavior, but\ndecided to search the list first.\n\nYeah, I smiled at\n\n Expanding reachable commits in commit graph: 139276% (870481/625), done.\n\nwhen I did the \"git gc --prune=now\" on the kernel, and apparently\nactually looked at the noise for the first time in a while.\n\nSo I don't mind the bug, but I think this should be fixed. Because\nalmost 140 _thousand_ percent just isn't right.\n\nSo Ack on this patch. Looks like it is already in 'pu', but hasn't\nmade it into next or master yet.\n\n Linus\n\nOn Fri, Sep 6, 2019 at 10:04 PM Jeff King wrote:\n>\n> From: SZEDER Gábor \n>\n> Commit 49bbc57a57 (commit-graph write: emit a percentage for all\n> progress, 2019-01-19) was a bit overeager when it added progress\n> percentages/ [...]\n"}]}