{"thread":{"id":"55071","subject":"[PATCH] pretty: lazy-load commit data when expanding user-format","startedAt":"2021-01-28T19:59:35Z","lastAt":"2021-01-29T01:12:13Z","messageCount":5,"participants":["Jeff King","Ævar Arnfjörð Bjarmason","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"415521","messageId":"YBMXM83xCZvC5WyA@coredump.intra.peff.net","threadId":"55071","inReplyTo":null,"subject":"[PATCH] pretty: lazy-load commit data when expanding user-format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-28T19:57:39Z","receivedAt":"2021-01-28T19:59:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we expand a user-format, we try to avoid work that isn't necessary\nfor the output. For instance, we don't bother parsing the commit header\nuntil we know we need the author, subject, etc.\n\nBut we do always load the commit object's contents from disk, even if\nthe format doesn't require it (e.g., just \"%H\"). Traditionally this\ndidn't matter much, because we'd have loaded it as part of the traversal\nanyway, and we'd typically have those bytes attached to the commit\nstruct (or these days, cached in a commit-slab).\n\nBut when we have a commit-graph, we might easily get to the point of\npretty-printing a commit without ever having looked at the actual object\ncontents. We should push off that load (and reencoding) until we're\ncertain that it's needed.\n\nI think the results of p4205 show the advantage pretty clearly (we serve\nparent and tree oids out of the commit struct itself, so they benefit as\nwell):\n\n  # using git.git as the test repo\n  Test                          HEAD^             HEAD\n  ----------------------------------------------------------------------\n  4205.1: log with %H           0.40(0.39+0.01)   0.03(0.02+0.01) -92.5%\n  4205.2: log with %h           0.45(0.44+0.01)   0.09(0.09+0.00) -80.0%\n  4205.3: log with %T           0.40(0.39+0.00)   0.04(0.04+0.00) -90.0%\n  4205.4: log with %t           0.46(0.46+0.00)   0.09(0.08+0.01) -80.4%\n  4205.5: log with %P           0.39(0.39+0.00)   0.03(0.03+0.00) -92.3%\n  4205.6: log with %p           0.46(0.46+0.00)   0.10(0.09+0.00) -78.3%\n  4205.7: log with %h-%h-%h     0.52(0.51+0.01)   0.15(0.14+0.00) -71.2%\n  4205.8: log with %an-%ae-%s   0.42(0.41+0.00)   0.42(0.41+0.01) +0.0%\n\n  # using linux.git as the test repo\n  Test                          HEAD^             HEAD\n  ----------------------------------------------------------------------\n  4205.1: log with %H           7.12(6.97+0.14)   0.76(0.65+0.11) -89.3%\n  4205.2: log with %h           7.35(7.19+0.16)   1.30(1.19+0.11) -82.3%\n  4205.3: log with %T           7.58(7.42+0.15)   1.02(0.94+0.08) -86.5%\n  4205.4: log with %t           8.05(7.89+0.15)   1.55(1.41+0.13) -80.7%\n  4205.5: log with %P           7.12(7.01+0.10)   0.76(0.69+0.07) -89.3%\n  4205.6: log with %p           7.38(7.27+0.10)   1.32(1.20+0.12) -82.1%\n  4205.7: log with %h-%h-%h     7.81(7.67+0.13)   1.79(1.67+0.12) -77.1%\n  4205.8: log with %an-%ae-%s   7.90(7.74+0.15)   7.81(7.66+0.15) -1.1%\n\nI added the final test to show where we don't improve (the 1% there is\njust lucky noise), but also as a regression test to make sure we're not\ndoing anything stupid like loading the commit multiple times when there\nare several placeholders that need it.\n\nReported-by: Michael Haggerty <mhagger@alum.mit.edu>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis benefits \"rev-list --format=\", as well, though there you can also\nuse things like \"--parents\" instead, which are already fast.\n\n pretty.c                           | 23 ++++++++++++-----------\n t/perf/p4205-log-pretty-formats.sh |  2 +-\n 2 files changed, 13 insertions(+), 12 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 3922f6f9f2..b4ff3f602f 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -783,6 +783,7 @@ enum trunc_type {\n };\n \n struct format_commit_context {\n+\tstruct repository *repository;\n \tconst struct commit *commit;\n \tconst struct pretty_print_context *pretty_ctx;\n \tunsigned commit_header_parsed:1;\n@@ -1373,10 +1374,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\treturn 2;\n \t}\n \n-\n \t/* For the rest we have to parse the commit header. */\n-\tif (!c->commit_header_parsed)\n+\tif (!c->commit_header_parsed) {\n+\t\tmsg = c->message =\n+\t\t\trepo_logmsg_reencode(c->repository, commit,\n+\t\t\t\t\t     &c->commit_encoding, \"UTF-8\");\n \t\tparse_commit_header(c);\n+\t}\n \n \tswitch (placeholder[0]) {\n \tcase 'a':\t/* author ... */\n@@ -1667,25 +1671,22 @@ void repo_format_commit_message(struct repository *r,\n \t\t\t\tconst struct pretty_print_context *pretty_ctx)\n {\n \tstruct format_commit_context context = {\n+\t\t.repository = r,\n \t\t.commit = commit,\n \t\t.pretty_ctx = pretty_ctx,\n \t\t.wrap_start = sb->len\n \t};\n \tconst char *output_enc = pretty_ctx->output_encoding;\n \tconst char *utf8 = \"UTF-8\";\n \n-\t/*\n-\t * convert a commit message to UTF-8 first\n-\t * as far as 'format_commit_item' assumes it in UTF-8\n-\t */\n-\tcontext.message = repo_logmsg_reencode(r, commit,\n-\t\t\t\t\t       &context.commit_encoding,\n-\t\t\t\t\t       utf8);\n-\n \tstrbuf_expand(sb, format, format_commit_item, &context);\n \trewrap_message_tail(sb, &context, 0, 0, 0);\n \n-\t/* then convert a commit message to an actual output encoding */\n+\t/*\n+\t * Convert output to an actual output encoding; note that\n+\t * format_commit_item() will always use UTF-8, so we don't\n+\t * have to bother if that's what the output wants.\n+\t */\n \tif (output_enc) {\n \t\tif (same_encoding(utf8, output_enc))\n \t\t\toutput_enc = NULL;\ndiff --git a/t/perf/p4205-log-pretty-formats.sh b/t/perf/p4205-log-pretty-formats.sh\nindex 7c26f4f337..609fecd65d 100755\n--- a/t/perf/p4205-log-pretty-formats.sh\n+++ b/t/perf/p4205-log-pretty-formats.sh\n@@ -6,7 +6,7 @@ test_description='Tests the performance of various pretty format placeholders'\n \n test_perf_default_repo\n \n-for format in %H %h %T %t %P %p %h-%h-%h\n+for format in %H %h %T %t %P %p %h-%h-%h %an-%ae-%s\n do\n \ttest_perf \"log with $format\" \"\n \t\tgit log --format=\\\"$format\\\" >/dev/null\n-- \n2.30.0.757.g41d9b3dd9b\n"},{"id":"415531","messageId":"87eei4pu3c.fsf@evledraar.gmail.com","threadId":"55071","inReplyTo":"YBMXM83xCZvC5WyA@coredump.intra.peff.net","subject":"Re: [PATCH] pretty: lazy-load commit data when expanding user-format","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-01-28T22:36:23Z","receivedAt":"2021-01-28T22:37:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jan 28 2021, Jeff King wrote:\n\n>   # using git.git as the test repo\n>   Test                          HEAD^             HEAD\n>   ----------------------------------------------------------------------\n>   4205.1: log with %H           0.40(0.39+0.01)   0.03(0.02+0.01) -92.5%\n>   4205.2: log with %h           0.45(0.44+0.01)   0.09(0.09+0.00) -80.0%\n>   4205.3: log with %T           0.40(0.39+0.00)   0.04(0.04+0.00) -90.0%\n>   4205.4: log with %t           0.46(0.46+0.00)   0.09(0.08+0.01) -80.4%\n>   4205.5: log with %P           0.39(0.39+0.00)   0.03(0.03+0.00) -92.3%\n>   4205.6: log with %p           0.46(0.46+0.00)   0.10(0.09+0.00) -78.3%\n>   4205.7: log with %h-%h-%h     0.52(0.51+0.01)   0.15(0.14+0.00) -71.2%\n>   4205.8: log with %an-%ae-%s   0.42(0.41+0.00)   0.42(0.41+0.01) +0.0%\n\nLooks nice!\n\n> diff --git a/t/perf/p4205-log-pretty-formats.sh b/t/perf/p4205-log-pretty-formats.sh\n> index 7c26f4f337..609fecd65d 100755\n> --- a/t/perf/p4205-log-pretty-formats.sh\n> +++ b/t/perf/p4205-log-pretty-formats.sh\n> @@ -6,7 +6,7 @@ test_description='Tests the performance of various pretty format placeholders'\n>  \n>  test_perf_default_repo\n>  \n> -for format in %H %h %T %t %P %p %h-%h-%h\n> +for format in %H %h %T %t %P %p %h-%h-%h %an-%ae-%s\n>  do\n>  \ttest_perf \"log with $format\" \"\n>  \t\tgit log --format=\\\"$format\\\" >/dev/null\n\n\nWhile we're at it it would be nice to have a few more formats that have\nto do with the body in some way in those tests, and stess things like\nmailmap/trailers etc.\n\n    %s\n    %b\n    %B\n    %N\n    %aN-%aE\n    %cn-%ce\n    %cN-%cE\n    %d\n    %D\n    %(trailers)\n\nJust paging over the git-log manpage, that seems to stress most of the\ncodepaths, i.e. subject/body, but also things like notes, .mailmap, ref\nnames, and body parsing (trailers).\n"},{"id":"415533","messageId":"YBNBbgZ/FLW8YVOe@nand.local","threadId":"55071","inReplyTo":"YBMXM83xCZvC5WyA@coredump.intra.peff.net","subject":"Re: [PATCH] pretty: lazy-load commit data when expanding user-format","fromName":"Taylor Blau","fromEmail":"ttaylorr@github.com","sentAt":"2021-01-28T22:58:18Z","receivedAt":"2021-01-28T22:59:19Z","isPatch":true,"sender":{"key":"ttaylorr@github.com","avatar":"https://gravatar.com/avatar/d5f3476f26b6f99cbb6b467e7ed7482f5762c8157bc73f569196e428bdcbea25?d=mp&s=160"},"body":"On Thu, Jan 28, 2021 at 02:57:39PM -0500, Jeff King wrote:\n> Reported-by: Michael Haggerty <mhagger@alum.mit.edu>\n> Signed-off-by: Jeff King <peff@peff.net>\n\nThis is delightful.\n\nAs I was reading the discussion you and Michael had off-list, I was\nworried that this would be more complicated than it ended up being. But,\nit's terrific to see that this change appeared to be relatively easy,\nand the performance speaks for itself.\n\nThanks for such a pleasant read!\n\nThanks,\nTaylor\n"},{"id":"415534","messageId":"xmqqlfccve2s.fsf@gitster.c.googlers.com","threadId":"55071","inReplyTo":"YBMXM83xCZvC5WyA@coredump.intra.peff.net","subject":"Re: [PATCH] pretty: lazy-load commit data when expanding user-format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-28T23:25:47Z","receivedAt":"2021-01-28T23:26:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I added the final test to show where we don't improve (the 1% there is\n> just lucky noise), but also as a regression test to make sure we're not\n> doing anything stupid like loading the commit multiple times when there\n> are several placeholders that need it.\n>\n> Reported-by: Michael Haggerty <mhagger@alum.mit.edu>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> This benefits \"rev-list --format=\", as well, though there you can also\n> use things like \"--parents\" instead, which are already fast.\n\nQuite pleased to see more work lazily done (and personally I am\nhappy to see Michael's name on the list---say Hi, I missed him).\n\n"},{"id":"415547","messageId":"YBNgq++oV3HorxVM@coredump.intra.peff.net","threadId":"55071","inReplyTo":"87eei4pu3c.fsf@evledraar.gmail.com","subject":"Re: [PATCH] pretty: lazy-load commit data when expanding user-format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-29T01:11:07Z","receivedAt":"2021-01-29T01:12:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 28, 2021 at 11:36:23PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > -for format in %H %h %T %t %P %p %h-%h-%h\n> > +for format in %H %h %T %t %P %p %h-%h-%h %an-%ae-%s\n> >  do\n> >  \ttest_perf \"log with $format\" \"\n> >  \t\tgit log --format=\\\"$format\\\" >/dev/null\n> \n> \n> While we're at it it would be nice to have a few more formats that have\n> to do with the body in some way in those tests, and stess things like\n> mailmap/trailers etc.\n> \n>     %s\n>     %b\n>     %B\n>     %N\n>     %aN-%aE\n>     %cn-%ce\n>     %cN-%cE\n>     %d\n>     %D\n>     %(trailers)\n> \n> Just paging over the git-log manpage, that seems to stress most of the\n> codepaths, i.e. subject/body, but also things like notes, .mailmap, ref\n> names, and body parsing (trailers).\n\nI'd prefer not to do so in this patch, since most of those aren't\nproviding any new data.\n\nI don't mind _too_ much if you'd like to do so on top for general\nregression-testing, but I'm generally a bit hesitant to throw a lot of\nstuff into the perf suite without a sense of what it's measuring, just\nbecause it already takes forever to run. So for example, is the\ndifference between %aN-%aE and %cN-%cE worth spending 21 seconds of CPU\n(3 times the 7 seconds it takes to run on the kernel)?\n\nOne test to check mailmap performance, or one for trailers, seems like\nit might be more directed. I think the existing test is likewise a bit\nwasteful in checking %h _and_ %t _and_ %p), though at least it is now\nmuch faster after my patch. ;)\n\n(In the regular test suite, we of course should be covering all these\nfor correctness already, and I think we do).\n\n-Peff\n"}]}