{"thread":{"id":"10072","subject":"Big performance regression with --no-color option in git-log","startedAt":"2007-09-29T10:02:23Z","lastAt":"2007-09-29T22:11:31Z","messageCount":12,"participants":["Marco Costalba","Pierre Habouzit","David Kastrup","Linus Torvalds","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"54326","messageId":"e5bfff550709290302h6016c6e8mf9d5dfddce07d985@mail.gmail.com","threadId":"10072","inReplyTo":null,"subject":"Big performance regression with --no-color option in git-log","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2007-09-29T10:02:23Z","receivedAt":"2007-09-29T10:02:23Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On today git tree\n\nbash-3.1$ time git log HEAD > /dev/null\n1.31user 0.07system 0:01.39elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+2455minor)pagefaults 0swaps\n\nbash-3.1$ time git log --no-color HEAD > /dev/null\n5.01user 0.11system 0:05.16elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+6319minor)pagefaults 0swaps\n\nbash-3.1$ git --version\ngit version 1.5.3.2.124.g648db\n\n\nMarco\n"},{"id":"54329","messageId":"20070929110130.GC4216@artemis.corp","threadId":"10072","inReplyTo":"e5bfff550709290302h6016c6e8mf9d5dfddce07d985@mail.gmail.com","subject":"Re: Big performance regression with --no-color option in git-log","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-29T11:01:30Z","receivedAt":"2007-09-29T11:01:30Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Sat, Sep 29, 2007 at 10:02:23AM +0000, Marco Costalba wrote:\n> On today git tree\n\nhmmm interesting... Here is what happens with --no-color:\n\n 38708 33.2920 diff_tree\n 23908 20.5628 base_name_compare\n 18478 15.8926 decode_tree_entry\n 8310 7.1473 find_pack_entry_one\n 5654 4.8629 update_tree_entry\n 2748 2.3635 pretty_print_commit\n 1838 1.5808 get_one_line\n 1460 1.2557 .plt\n\nand wihtout it:\n\n 2594 18.0893 pretty_print_commit\n 1887 13.1590 get_one_line\n 1574 10.9763 find_pack_entry_one\n 967 6.7434 get_short_sha1\n 892 6.2204 parse_commit_buffer\n 863 6.0181 lookup_object\n 595 4.1492 strbuf_grow\n 440 3.0683 insert_obj_hash\n\nWhy on earth does --no-color has such a different code path ...\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"54330","messageId":"20070929110914.GA32142@artemis.corp","threadId":"10072","inReplyTo":"20070929110130.GC4216@artemis.corp","subject":"Re: Big performance regression with --no-color option in git-log","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-29T11:09:14Z","receivedAt":"2007-09-29T11:09:14Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Sat, Sep 29, 2007 at 11:01:30AM +0000, Pierre Habouzit wrote:\n> On Sat, Sep 29, 2007 at 10:02:23AM +0000, Marco Costalba wrote:\n> > On today git tree\n\n Okay for me it's not only from today, and the issue seems to be with\nthe fact that we pass options or that we don't... git log --color also\ntakes an awful lot of time here, whereas it's my default. Looks like\ndiff_opt_parse is called too many times.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"54331","messageId":"e5bfff550709290429n291968f2md8068a945ff7a79e@mail.gmail.com","threadId":"10072","inReplyTo":"20070929110914.GA32142@artemis.corp","subject":"Re: Big performance regression with --no-color option in git-log","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2007-09-29T11:29:30Z","receivedAt":"2007-09-29T11:29:30Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 9/29/07, Pierre Habouzit wrote:\n> On Sat, Sep 29, 2007 at 11:01:30AM +0000, Pierre Habouzit wrote:\n> > On Sat, Sep 29, 2007 at 10:02:23AM +0000, Marco Costalba wrote:\n> > > On today git tree\n>\n> Okay for me it's not only from today, and the issue seems to be with\n> the fact that we pass options or that we don't... git log --color also\n> takes an awful lot of time here, whereas it's my default. Looks like\n> diff_opt_parse is called too many times.\n>\n\nThere was a patch some time ago to fix a similar issue with '-z'\noption (66e41f7b9912), perhaps the fix could be the same also for\n--no-color, --color, or other new options that force to actually\ncompute diffs when is not needed.\n\nMarco\n"},{"id":"54332","messageId":"20070929123724.01BB045EC@madism.org","threadId":"10072","inReplyTo":"e5bfff550709290429n291968f2md8068a945ff7a79e@mail.gmail.com","subject":"[PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-29T12:35:56Z","receivedAt":"2007-09-29T12:35:56Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"Signed-off-by: Pierre Habouzit \n---\n\n revision.c | 5 +++--\n 1 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 33d092c..0dee835 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1209,8 +1209,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch\n \n \t\t\topts = diff_opt_parse(&revs->diffopt, argv+i, argc-i);\n \t\t\tif (opts > 0) {\n-\t\t\t\tif (strcmp(argv[i], \"-z\"))\n-\t\t\t\t\trevs->diff = 1;\n+\t\t\t\trevs->diff = strcmp(argv[i], \"-z\")\n+\t\t\t\t\t&& strcmp(argv[i], \"--color\")\n+\t\t\t\t\t&& strcmp(argv[i], \"--no-color\");\n \t\t\t\ti += opts - 1;\n \t\t\t\tcontinue;\n \t\t\t}\n-- \n1.5.3.2.1110.g61a7cd-dirty\n"},{"id":"54334","messageId":"85ve9t8wh7.fsf@lola.goethe.zz","threadId":"10072","inReplyTo":"20070929123724.01BB045EC@madism.org","subject":"Re: [PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed.","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2007-09-29T12:52:36Z","receivedAt":"2007-09-29T12:52:36Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Pierre Habouzit writes:\n\n> Signed-off-by: Pierre Habouzit \n\n> -\t\t\t\tif (strcmp(argv[i], \"-z\"))\n> -\t\t\t\t\trevs->diff = 1;\n> +\t\t\t\trevs->diff = strcmp(argv[i], \"-z\")\n> +\t\t\t\t\t&& strcmp(argv[i], \"--color\")\n> +\t\t\t\t\t&& strcmp(argv[i], \"--no-color\");\n> \t\t\t\ti += opts - 1;\n> \t\t\t\tcontinue;\n\nThis can clear a previously set value of revs->diff.\n\n-- \nDavid Kastrup, Kriemhildstr. 15, 44793 Bochum\n"},{"id":"54335","messageId":"20070929125633.GC32142@artemis.corp","threadId":"10072","inReplyTo":"85ve9t8wh7.fsf@lola.goethe.zz","subject":"Re: [PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-29T12:56:33Z","receivedAt":"2007-09-29T12:56:33Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Sat, Sep 29, 2007 at 12:52:36PM +0000, David Kastrup wrote:\n> Pierre Habouzit writes:\n> \n> > Signed-off-by: Pierre Habouzit \n> \n> > -\t\t\t\tif (strcmp(argv[i], \"-z\"))\n> > -\t\t\t\t\trevs->diff = 1;\n> > +\t\t\t\trevs->diff = strcmp(argv[i], \"-z\")\n> > +\t\t\t\t\t&& strcmp(argv[i], \"--color\")\n> > +\t\t\t\t\t&& strcmp(argv[i], \"--no-color\");\n> > \t\t\t\ti += opts - 1;\n> > \t\t\t\tcontinue;\n> \n> This can clear a previously set value of revs->diff.\n\n Good catch, that should be |= of course.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"54338","messageId":"e5bfff550709290606k71cc3c50h5ea42441223dede@mail.gmail.com","threadId":"10072","inReplyTo":"20070929125633.GC32142@artemis.corp","subject":"Re: [PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed.","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2007-09-29T13:06:16Z","receivedAt":"2007-09-29T13:06:16Z","isPatch":true,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 9/29/07, Pierre Habouzit wrote:\n> >\n> > > Signed-off-by: Pierre Habouzit \n> >\n> > > - if (strcmp(argv[i], \"-z\"))\n> > > - revs->diff = 1;\n> > > + revs->diff = strcmp(argv[i], \"-z\")\n> > > + && strcmp(argv[i], \"--color\")\n> > > + && strcmp(argv[i], \"--no-color\");\n> > > i += opts - 1;\n> > > continue;\n> >\n> > This can clear a previously set value of revs->diff.\n>\n> Good catch, that should be |= of course.\n>\n\nPerhaps also with an 'if' before to avoid clearing an already set\nrevs->diff if the passed option does not match any of the three.\n\nMarco\n"},{"id":"54339","messageId":"e5bfff550709290607y6704db5dgdddc4a3db30f067d@mail.gmail.com","threadId":"10072","inReplyTo":"e5bfff550709290606k71cc3c50h5ea42441223dede@mail.gmail.com","subject":"Re: [PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed.","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2007-09-29T13:07:39Z","receivedAt":"2007-09-29T13:07:39Z","isPatch":true,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 9/29/07, Marco Costalba wrote:\n>\n> Perhaps also with an 'if' before to avoid clearing an already set\n> revs->diff if the passed option does not match any of the three.\n>\n\nPlease discard, I mistakenly pressed 'send' button.\n"},{"id":"54348","messageId":"alpine.LFD.0.999.0709290932040.3579@woody.linux-foundation.org","threadId":"10072","inReplyTo":"20070929123724.01BB045EC@madism.org","subject":"Fix revision log diff setup, avoid unnecessary diff generation","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-09-29T16:50:39Z","receivedAt":"2007-09-29T16:50:39Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nWe used to incorrectly start calculating diffs whenever any argument but \n'-z' was recognized by the diff options parsing. That was bogus, since not \nall arguments result in diffs being needed, so we just waste a lot of time \nand effort on calculating diffs that don't matter.\n\nThis actually also fixes another bug in \"git log\". Try this:\n\n\tgit log -C\n\nand notice how it prints an extra empty line in between log entries, even \nthough it never prints the actual diff (because we didn't ask for any diff \nformat, so the diff machinery never prints anything).\n\nWith this patch, that bogus empty line is gone, because \"revs->diff\" is \nnever set. So this isn't just a \"wasted time and effort\" issue, it's also \na slight semantic fix.\n\nSigned-off-by: Linus Torvalds \n---\nOn Sat, 29 Sep 2007, Pierre Habouzit wrote:\n>\n> -\t\t\t\tif (strcmp(argv[i], \"-z\"))\n> -\t\t\t\t\trevs->diff = 1;\n> +\t\t\t\trevs->diff = strcmp(argv[i], \"-z\")\n> +\t\t\t\t\t&& strcmp(argv[i], \"--color\")\n> +\t\t\t\t\t&& strcmp(argv[i], \"--no-color\");\n\nThe old code was already pretty damn ugly, the new code is worse (never \nmind the bug).\n\nI don't think we should care *at*all* about the actual argument string, we \nshould just look at what the diffopts end up being at the end.\n\nSo I would suggest a patch like the appended instead!\n\n(Maybe there are other cases where we'd want to run the diff, but I can't \nthink of any)\n\n\t\tLinus\n\n---\n revision.c | 10 ++++++++--\n 1 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 33d092c..6584713 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1209,8 +1209,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch\n \n \t\t\topts = diff_opt_parse(&revs->diffopt, argv+i, argc-i);\n \t\t\tif (opts > 0) {\n-\t\t\t\tif (strcmp(argv[i], \"-z\"))\n-\t\t\t\t\trevs->diff = 1;\n \t\t\t\ti += opts - 1;\n \t\t\t\tcontinue;\n \t\t\t}\n@@ -1254,6 +1252,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch\n \t\tadd_pending_object_with_mode(revs, object, def, mode);\n \t}\n \n+\t/* Did the user ask for any diff output? Run the diff! */\n+\tif (revs->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT)\n+\t\trevs->diff = 1;\n+\n+\t/* Pickaxe needs diffs */\n+\tif (revs->diffopt.pickaxe)\n+\t\trevs->diff = 1;\n+\n \tif (revs->topo_order)\n \t\trevs->limited = 1;\n \n"},{"id":"54350","messageId":"7v4phdl808.fsf@gitster.siamese.dyndns.org","threadId":"10072","inReplyTo":"20070929123724.01BB045EC@madism.org","subject":"Re: [PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-29T17:02:47Z","receivedAt":"2007-09-29T17:02:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pierre Habouzit writes:\n\n> Signed-off-by: Pierre Habouzit \n> ---\n>\n> revision.c | 5 +++--\n> 1 files changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/revision.c b/revision.c\n> index 33d092c..0dee835 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1209,8 +1209,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch\n> \n> \t\t\topts = diff_opt_parse(&revs->diffopt, argv+i, argc-i);\n> \t\t\tif (opts > 0) {\n> -\t\t\t\tif (strcmp(argv[i], \"-z\"))\n> -\t\t\t\t\trevs->diff = 1;\n> +\t\t\t\trevs->diff = strcmp(argv[i], \"-z\")\n> +\t\t\t\t\t&& strcmp(argv[i], \"--color\")\n> +\t\t\t\t\t&& strcmp(argv[i], \"--no-color\");\n> \t\t\t\ti += opts - 1;\n> \t\t\t\tcontinue;\n> \t\t\t}\n\nAside from the \"don't override the option that is already set\"\ncomment from David, I am somewhat unhappy that this piece\nalready knows too much about which option to diff potentially\nchanges the output (but not commits us to produce the diff) and\nwhich option causes us to actually produce output.\n"},{"id":"54369","messageId":"7vve9tjf58.fsf@gitster.siamese.dyndns.org","threadId":"10072","inReplyTo":"alpine.LFD.0.999.0709290932040.3579@woody.linux-foundation.org","subject":"Re: Fix revision log diff setup, avoid unnecessary diff generation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-29T22:11:31Z","receivedAt":"2007-09-29T22:11:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Sat, 29 Sep 2007, Pierre Habouzit wrote:\n>>\n>> -\t\t\t\tif (strcmp(argv[i], \"-z\"))\n>> -\t\t\t\t\trevs->diff = 1;\n>> +\t\t\t\trevs->diff = strcmp(argv[i], \"-z\")\n>> +\t\t\t\t\t&& strcmp(argv[i], \"--color\")\n>> +\t\t\t\t\t&& strcmp(argv[i], \"--no-color\");\n>\n> The old code was already pretty damn ugly, the new code is worse (never \n> mind the bug).\n>\n> I don't think we should care *at*all* about the actual argument string, we \n> should just look at what the diffopts end up being at the end.\n\nThanks, I think this is the sanest thing to do.\n"}]}