# Big performance regression with --no-color option in git-log 12 messages from 2007-09-29 to 2007-09-29. Participants: Marco Costalba, Pierre Habouzit, David Kastrup, Linus Torvalds, Junio C Hamano. Thread: https://gitlist.dev/t/10072 ## Marco Costalba, 2007-09-29 10:02 Subject: Big performance regression with --no-color option in git-log Message-ID: URL: https://gitlist.dev/e/e5bfff550709290302h6016c6e8mf9d5dfddce07d985%40mail.gmail.com ``` On today git tree bash-3.1$ time git log HEAD > /dev/null 1.31user 0.07system 0:01.39elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k 0inputs+0outputs (0major+2455minor)pagefaults 0swaps bash-3.1$ time git log --no-color HEAD > /dev/null 5.01user 0.11system 0:05.16elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k 0inputs+0outputs (0major+6319minor)pagefaults 0swaps bash-3.1$ git --version git version 1.5.3.2.124.g648db Marco ``` ## Pierre Habouzit, 2007-09-29 11:01 Subject: Re: Big performance regression with --no-color option in git-log Message-ID: <20070929110130.GC4216@artemis.corp> URL: https://gitlist.dev/e/20070929110130.GC4216%40artemis.corp In-Reply-To: ``` On Sat, Sep 29, 2007 at 10:02:23AM +0000, Marco Costalba wrote: > On today git tree hmmm interesting... Here is what happens with --no-color: 38708 33.2920 diff_tree 23908 20.5628 base_name_compare 18478 15.8926 decode_tree_entry 8310 7.1473 find_pack_entry_one 5654 4.8629 update_tree_entry 2748 2.3635 pretty_print_commit 1838 1.5808 get_one_line 1460 1.2557 .plt and wihtout it: 2594 18.0893 pretty_print_commit 1887 13.1590 get_one_line 1574 10.9763 find_pack_entry_one 967 6.7434 get_short_sha1 892 6.2204 parse_commit_buffer 863 6.0181 lookup_object 595 4.1492 strbuf_grow 440 3.0683 insert_obj_hash Why on earth does --no-color has such a different code path ... -- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org ``` ## Pierre Habouzit, 2007-09-29 11:09 Subject: Re: Big performance regression with --no-color option in git-log Message-ID: <20070929110914.GA32142@artemis.corp> URL: https://gitlist.dev/e/20070929110914.GA32142%40artemis.corp In-Reply-To: <20070929110130.GC4216@artemis.corp> ``` On Sat, Sep 29, 2007 at 11:01:30AM +0000, Pierre Habouzit wrote: > On Sat, Sep 29, 2007 at 10:02:23AM +0000, Marco Costalba wrote: > > On today git tree Okay for me it's not only from today, and the issue seems to be with the fact that we pass options or that we don't... git log --color also takes an awful lot of time here, whereas it's my default. Looks like diff_opt_parse is called too many times. -- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org ``` ## Marco Costalba, 2007-09-29 11:29 Subject: Re: Big performance regression with --no-color option in git-log Message-ID: URL: https://gitlist.dev/e/e5bfff550709290429n291968f2md8068a945ff7a79e%40mail.gmail.com In-Reply-To: <20070929110914.GA32142@artemis.corp> ``` On 9/29/07, Pierre Habouzit wrote: > On Sat, Sep 29, 2007 at 11:01:30AM +0000, Pierre Habouzit wrote: > > On Sat, Sep 29, 2007 at 10:02:23AM +0000, Marco Costalba wrote: > > > On today git tree > > Okay for me it's not only from today, and the issue seems to be with > the fact that we pass options or that we don't... git log --color also > takes an awful lot of time here, whereas it's my default. Looks like > diff_opt_parse is called too many times. > There was a patch some time ago to fix a similar issue with '-z' option (66e41f7b9912), perhaps the fix could be the same also for --no-color, --color, or other new options that force to actually compute diffs when is not needed. Marco ``` ## Pierre Habouzit, 2007-09-29 12:35 Subject: [PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed. Message-ID: <20070929123724.01BB045EC@madism.org> URL: https://gitlist.dev/e/20070929123724.01BB045EC%40madism.org In-Reply-To: ``` Signed-off-by: Pierre Habouzit --- revision.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/revision.c b/revision.c index 33d092c..0dee835 100644 --- a/revision.c +++ b/revision.c @@ -1209,8 +1209,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch opts = diff_opt_parse(&revs->diffopt, argv+i, argc-i); if (opts > 0) { - if (strcmp(argv[i], "-z")) - revs->diff = 1; + revs->diff = strcmp(argv[i], "-z") + && strcmp(argv[i], "--color") + && strcmp(argv[i], "--no-color"); i += opts - 1; continue; } -- 1.5.3.2.1110.g61a7cd-dirty ``` ## David Kastrup, 2007-09-29 12:52 Subject: Re: [PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed. Message-ID: <85ve9t8wh7.fsf@lola.goethe.zz> URL: https://gitlist.dev/e/85ve9t8wh7.fsf%40lola.goethe.zz In-Reply-To: <20070929123724.01BB045EC@madism.org> ``` Pierre Habouzit writes: > Signed-off-by: Pierre Habouzit > - if (strcmp(argv[i], "-z")) > - revs->diff = 1; > + revs->diff = strcmp(argv[i], "-z") > + && strcmp(argv[i], "--color") > + && strcmp(argv[i], "--no-color"); > i += opts - 1; > continue; This can clear a previously set value of revs->diff. -- David Kastrup, Kriemhildstr. 15, 44793 Bochum ``` ## Pierre Habouzit, 2007-09-29 12:56 Subject: Re: [PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed. Message-ID: <20070929125633.GC32142@artemis.corp> URL: https://gitlist.dev/e/20070929125633.GC32142%40artemis.corp In-Reply-To: <85ve9t8wh7.fsf@lola.goethe.zz> ``` On Sat, Sep 29, 2007 at 12:52:36PM +0000, David Kastrup wrote: > Pierre Habouzit writes: > > > Signed-off-by: Pierre Habouzit > > > - if (strcmp(argv[i], "-z")) > > - revs->diff = 1; > > + revs->diff = strcmp(argv[i], "-z") > > + && strcmp(argv[i], "--color") > > + && strcmp(argv[i], "--no-color"); > > i += opts - 1; > > continue; > > This can clear a previously set value of revs->diff. Good catch, that should be |= of course. -- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org ``` ## Marco Costalba, 2007-09-29 13:06 Subject: Re: [PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed. Message-ID: URL: https://gitlist.dev/e/e5bfff550709290606k71cc3c50h5ea42441223dede%40mail.gmail.com In-Reply-To: <20070929125633.GC32142@artemis.corp> ``` On 9/29/07, Pierre Habouzit wrote: > > > > > Signed-off-by: Pierre Habouzit > > > > > - if (strcmp(argv[i], "-z")) > > > - revs->diff = 1; > > > + revs->diff = strcmp(argv[i], "-z") > > > + && strcmp(argv[i], "--color") > > > + && strcmp(argv[i], "--no-color"); > > > i += opts - 1; > > > continue; > > > > This can clear a previously set value of revs->diff. > > Good catch, that should be |= of course. > Perhaps also with an 'if' before to avoid clearing an already set revs->diff if the passed option does not match any of the three. Marco ``` ## Marco Costalba, 2007-09-29 13:07 Subject: Re: [PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed. Message-ID: URL: https://gitlist.dev/e/e5bfff550709290607y6704db5dgdddc4a3db30f067d%40mail.gmail.com In-Reply-To: ``` On 9/29/07, Marco Costalba wrote: > > Perhaps also with an 'if' before to avoid clearing an already set > revs->diff if the passed option does not match any of the three. > Please discard, I mistakenly pressed 'send' button. ``` ## Linus Torvalds, 2007-09-29 16:50 Subject: Fix revision log diff setup, avoid unnecessary diff generation Message-ID: URL: https://gitlist.dev/e/alpine.LFD.0.999.0709290932040.3579%40woody.linux-foundation.org In-Reply-To: <20070929123724.01BB045EC@madism.org> ``` We used to incorrectly start calculating diffs whenever any argument but '-z' was recognized by the diff options parsing. That was bogus, since not all arguments result in diffs being needed, so we just waste a lot of time and effort on calculating diffs that don't matter. This actually also fixes another bug in "git log". Try this: git log -C and notice how it prints an extra empty line in between log entries, even though it never prints the actual diff (because we didn't ask for any diff format, so the diff machinery never prints anything). With this patch, that bogus empty line is gone, because "revs->diff" is never set. So this isn't just a "wasted time and effort" issue, it's also a slight semantic fix. Signed-off-by: Linus Torvalds --- On Sat, 29 Sep 2007, Pierre Habouzit wrote: > > - if (strcmp(argv[i], "-z")) > - revs->diff = 1; > + revs->diff = strcmp(argv[i], "-z") > + && strcmp(argv[i], "--color") > + && strcmp(argv[i], "--no-color"); The old code was already pretty damn ugly, the new code is worse (never mind the bug). I don't think we should care *at*all* about the actual argument string, we should just look at what the diffopts end up being at the end. So I would suggest a patch like the appended instead! (Maybe there are other cases where we'd want to run the diff, but I can't think of any) Linus --- revision.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-) diff --git a/revision.c b/revision.c index 33d092c..6584713 100644 --- a/revision.c +++ b/revision.c @@ -1209,8 +1209,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch opts = diff_opt_parse(&revs->diffopt, argv+i, argc-i); if (opts > 0) { - if (strcmp(argv[i], "-z")) - revs->diff = 1; i += opts - 1; continue; } @@ -1254,6 +1252,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch add_pending_object_with_mode(revs, object, def, mode); } + /* Did the user ask for any diff output? Run the diff! */ + if (revs->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT) + revs->diff = 1; + + /* Pickaxe needs diffs */ + if (revs->diffopt.pickaxe) + revs->diff = 1; + if (revs->topo_order) revs->limited = 1; ``` ## Junio C Hamano, 2007-09-29 17:02 Subject: Re: [PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed. Message-ID: <7v4phdl808.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v4phdl808.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20070929123724.01BB045EC@madism.org> ``` Pierre Habouzit writes: > Signed-off-by: Pierre Habouzit > --- > > revision.c | 5 +++-- > 1 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/revision.c b/revision.c > index 33d092c..0dee835 100644 > --- a/revision.c > +++ b/revision.c > @@ -1209,8 +1209,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch > > opts = diff_opt_parse(&revs->diffopt, argv+i, argc-i); > if (opts > 0) { > - if (strcmp(argv[i], "-z")) > - revs->diff = 1; > + revs->diff = strcmp(argv[i], "-z") > + && strcmp(argv[i], "--color") > + && strcmp(argv[i], "--no-color"); > i += opts - 1; > continue; > } Aside from the "don't override the option that is already set" comment from David, I am somewhat unhappy that this piece already knows too much about which option to diff potentially changes the output (but not commits us to produce the diff) and which option causes us to actually produce output. ``` ## Junio C Hamano, 2007-09-29 22:11 Subject: Re: Fix revision log diff setup, avoid unnecessary diff generation Message-ID: <7vve9tjf58.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vve9tjf58.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Linus Torvalds writes: > On Sat, 29 Sep 2007, Pierre Habouzit wrote: >> >> - if (strcmp(argv[i], "-z")) >> - revs->diff = 1; >> + revs->diff = strcmp(argv[i], "-z") >> + && strcmp(argv[i], "--color") >> + && strcmp(argv[i], "--no-color"); > > The old code was already pretty damn ugly, the new code is worse (never > mind the bug). > > I don't think we should care *at*all* about the actual argument string, we > should just look at what the diffopts end up being at the end. Thanks, I think this is the sanest thing to do. ```