[PATCH 2/3 (edit v2)] gitweb: Cache $parent_commit info in git_blame()

From: Jakub Narebski <jnareb@gmail.com>
Date: Dec 11, 2008, 00:33 UTC
Message-ID: <200812110133.33124.jnareb@gmail.com>
In-Reply-To: <7v7i67zsgj.fsf@gitster.siamese.dyndns.org>

Luben Tuikov changed 'lineno' link from leading to commit which gave current version of given block of lines, to leading to parent of this commit in 244a70e (Blame "linenr" link jumps to previous state at "orig_lineno"). This made possible data mining using 'blame' view.

The current implementation calls rev-parse once per each blamed line to find parent revision of blamed commit, even when the same commit appears more than once, which is inefficient.

This patch attempts to mitigate this issue by storing (caching) $parent_commit info in %metainfo, which makes gitweb call git-rev-parse only once per each unique commit in blame output.

In the tables below you can see simple benchmark comparing gitweb performance before and after this patch

File | L[1] | C[2] || Time0[3] | Before[4] | After[4] ==================================================================== blob.h | 18 | 4 || 0m1.727s | 0m2.545s | 0m2.474s GIT-VERSION-GEN | 42 | 13 || 0m2.165s | 0m2.448s | 0m2.071s README | 46 | 6 || 0m1.593s | 0m2.727s | 0m2.242s revision.c | 1923 | 121 || 0m2.357s | 0m30.365s | 0m7.028s gitweb/gitweb.perl | 6291 | 428 || 0m8.080s | 1m37.244s | 0m20.627s

File | L/C | Before/After ========================================= blob.h | 4.5 | 1.03 GIT-VERSION-GEN | 3.2 | 1.18 README | 7.7 | 1.22 revision.c | 15.9 | 4.32 gitweb/gitweb.perl | 14.7 | 4.71

As you can see the greater ratio of lines in file to unique commits in blame output, the greater gain from the new implementation.

Footnotes:
~~~~~~~~~~
[1] Lines:
    $ wc -l <file>
[2] Individual commits in blame output:
    $ git blame -p <file> | grep author-time | wc -l
[3] Time for running "git blame -p" (user time, single run):
    $ time git blame -p <file> >/dev/null
[4] Time to run gitweb as Perl script from command line:
    $ gitweb-run.sh "p=.git;a=blame;f=<file>" > /dev/null 2>&1

The gitweb-run.sh script includes slightly modified (with adjusted pathnames) code from gitweb_run() function from the test script t/t9500-gitweb-standalone-no-errors.sh; gitweb config file gitweb_config.perl contents (again up to adjusting pathnames; in particular $projectroot variable should point to top directory of git repository) can be found in the same place.

Alternate solutions: ~~~~~~~~~~~~~~~~~~~~ Alternate solution would be to open bidi pipe to "git cat-file --batch-check", (like in Git::Repo in gitweb caching by Lea Wiemann), feed $long_rev^ to it, and parse its output which has the following form:

  926b07e694599d86cec668475071b32147c95034 commit 637

This would mean one call to git-cat-file for the whole 'blame' view, instead of one call to git-rev-parse per each unique commit in blame output.

Yet another solution would be to change use of validate_refname() to validate_revision() when checking script parameters (CGI query or path_info), with validate_revision being something like the following:

  sub validate_revision {
        my $rev = shift;
        return validate_refname(strip_rev_suffixes($rev));
  }

so we don't need to calculate $long_rev^, but can pass "$long_rev^" as 'hb' parameter.

This solution has the advantage that it can be easily adapted to future incremental blame output.

Acked-by: Luben Tuikov <ltuikov@yahoo.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
On Wed, 10 Dec 2008, Junio C Hamano wrote:

Show 10 quoted lines

> To recap, I think the commit log for this patch would have been much
> easier to read if it were presented in this order:
> 
>         a paragraph to establish the context;
> 
>         a paragraph to state what problem it tries to solve;
> 
>         a paragraph (or more) to explain the solution; and finally
> 
>         a paragraph to discuss possible future enhancements.

Like this?

Only commit message has changed.

 gitweb/gitweb.perl |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 1b800f4..916396a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4657,11 +4657,17 @@ HTML
 			              esc_html($rev));
 			print "</td>\n";
 		}
-		open (my $dd, "-|", git_cmd(), "rev-parse", "$full_rev^")
-			or die_error(500, "Open git-rev-parse failed");
-		my $parent_commit = <$dd>;
-		close $dd;
-		chomp($parent_commit);
+		my $parent_commit;
+		if (!exists $meta->{'parent'}) {
+			open (my $dd, "-|", git_cmd(), "rev-parse", "$full_rev^")
+				or die_error(500, "Open git-rev-parse failed");
+			$parent_commit = <$dd>;
+			close $dd;
+			chomp($parent_commit);
+			$meta->{'parent'} = $parent_commit;
+		} else {
+			$parent_commit = $meta->{'parent'};
+		}
 		my $blamed = href(action => 'blame',
 		                  file_name => $meta->{'filename'},
 		                  hash_base => $parent_commit);

-- 
1.6.0.4