threads / discuss / 10072
Subject: Big performance regression with --no-color option in git-log
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 HabouzitRe: Big performance regression with --no-color option in git-log
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 .pltand 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_hashWhy on earth does --no-color has such a different code path ...
-- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org
Pierre HabouzitRe: Big performance regression with --no-color option in git-log
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
Re: Big performance regression with --no-color option in git-log
On 9/29/07, Pierre Habouzit <madcoder@debian.org> 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[PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed.
Signed-off-by: Pierre Habouzit <madcoder@debian.org> ---
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 KastrupRe: [PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed.
Pierre Habouzit <madcoder@debian.org> writes:
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> - 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 HabouzitRe: [PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed.
On Sat, Sep 29, 2007 at 12:52:36PM +0000, David Kastrup wrote:
> Pierre Habouzit <madcoder@debian.org> writes: > > > Signed-off-by: Pierre Habouzit <madcoder@debian.org> > > > - 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
Re: [PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed.
On 9/29/07, Pierre Habouzit <madcoder@debian.org> wrote:
> > > > > Signed-off-by: Pierre Habouzit <madcoder@debian.org> > > > > > - 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
Re: [PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed.
On 9/29/07, Marco Costalba <mcostalba@gmail.com> 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 TorvaldsFix revision log diff setup, avoid unnecessary diff generation
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 <torvalds@linux-foundation.org> --- 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 HamanoRe: Fix revision log diff setup, avoid unnecessary diff generation
Linus Torvalds <torvalds@linux-foundation.org> 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.
Junio C HamanoRe: [PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed.
Pierre Habouzit <madcoder@debian.org> writes:
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
>
> 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.