Big performance regression with --no-color option in git-log

Big performance regression with --no-color option in git-log

[Marco Costalba]

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

Re: Big performance regression with --no-color option in git-log

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1034 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Big performance regression with --no-color option in git-log

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 593 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Big performance regression with --no-color option in git-log

[Marco Costalba]

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

[PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed.

[Pierre Habouzit]

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

Re: [PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed.

[David Kastrup]

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

Re: [PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 704 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed.

[Marco Costalba]

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.

[Marco Costalba]

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.

Fix revision log diff setup, avoid unnecessary diff generation

[Linus Torvalds]

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;
 

Re: Fix revision log diff setup, avoid unnecessary diff generation

[Junio C Hamano]

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.

Re: [PATCH 1/1] --color and --no-color git-log options don't need diffs to be computed.

[Junio C Hamano]

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.