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

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                                                [email protected]
OOO                                                http://www.madism.org

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                                                [email protected]
OOO                                                http://www.madism.org

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

---

 revision.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

index 33d092c..0dee835 100644
@@ -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

Good catch, that should be |= of course.

·O·  Pierre Habouzit
··O                                                [email protected]
OOO                                                http://www.madism.org

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

Please discard, I mistakenly pressed 'send' button.

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.

---

---
 revision.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

index 33d092c..6584713 100644
@@ -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;
 

Thanks, I think this is the sanest thing to do.