Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame

From: Jakub Narebski <jnareb@gmail.com>
Date: Jun 4, 2008, 00:11 UTC
Message-ID: <200806040211.29430.jnareb@gmail.com>
In-Reply-To: <4845CF9F.10604@gmail.com>

Lea Wiemann wrote:

Show 9 quoted lines

> Jakub Narebski wrote:
>>
>> I don't think %parent_commits hash is suitable for caching; it is only
>> intermediate step, reducing number of git command calls (and forks) [...]
>> 
>> ATTENTION! This example shows where caching [parsed] data have problems 
>> compared to front-end caching (caching output).
> 
> ATTENTION!  Could we please stop having this discussion?!

Yeah, yeah, I know. "Talk is cheap, show me the code" (or at least pseudocode).

> Your argument  
> is completely bogus.  If the parent commit hashes are in cache, it's an 
> almost zero-time cache lookup.

You have cut a bit too much (quoted a bit too little) for me to decide if I made myself clear wrt. saving %parent_commits hash into cache.

What I wanted to say that in caching intermediate data for 'blame' view you have to save to cache something like @blocks (or @lines) array. This array can contain parents of blamed commits, so there is no need for saving %parent_commits separately: it would be duplication of information. This hash is needed to reduce number of calls to git-rev-parse, and is used to generate parsed info, which info in turn (I think) can be cached.

> The only difference it might make  
> compared front-end caching is the CPU time it takes to generate the 
> page, and *I want to see benchmarks before I even start thinking about 
> CPU*.  Okay?  Good, thanks.

The only place where I think front-end caching could be better is 'blob' view with syntax highlighting (using some external filter, like GNU Source Highlight)... which is not implemented yet.

I thought that snapshots (if enabled) would fall in this category, but this is the case where data cache is almost identical to output cache (the same happens for [almost] all "raw" / *_plain views).

Show 12 quoted lines

> Sorry I'm a little indignant, but you seem to be somehow trying to tell 
> me what to implement, and that gets annoying after a while.  I don't 
> mind your input, but at some point the discussion just doesn't go any 
> further.
> 
>> Problems occur when we try to cache page with _streaming_ output, such 
>> as blob view, blame view, diff part of commitdiff etc.
> 
> We can still stream backend-cache-backed data, though it's a little 
> harder.  It's mostly a memory, not a performance issue though -- the 
> only point where I think it actually would be performance-relevant is 
> blame, and blame doesn't stream anyway (see below).

And snapshots. We certainly want to stream snapshots, as they can be quite large.

Also blob_plain view might be difficult, if there are extremely large binary files in the repository (it should not happen often, but it can happen).

[...]

Show 8 quoted lines

>>> 2) Major point: You're still forking a lot.  The Right Thing is to
>>> condense everything into a single call
>> 
>> This is not a good solution for 'blame' view, which is generated "on the 
>> fly", by streaming git-blame output via filter.
> 
> No, whether you have your "while <$fd>" loop or not doesn't make a 
> difference.

It perhaps makes no difference performance wise (solution with "git rev-list --parents --no-walk" has one fork more), but it might make code unnecessarily more complicated. In the rev-list solution you have to browse git-blame output to gather all blamed commits one want to find parents of; in the case of extending git-blame you can just process block after block of code.

> Blame first calculates the whole blame and then dumps it  
> out in zero-time, unless you use --incremental.

There is some code in the mailing list archive (and perhaps used by repo's gitweb, but I might be mistaken), which adds git_blame_incremental and use AJAX together with "git blame --incremental" to reduce latency. It was done by having JavaScript check if browser is AJAX-capable, and if it was rewriting 'blame' links to 'blame_incremental'. But if there exist cached blame, I think it would be as fast (in terms of latency) to generate 'blame' from cache as to generate 'blame_incremental'.

> So there's no  
> performance difference in getting all blame output and then dumping it 
> out vs. reading and outputting it line-by-line.

Performance wise, perhaps not. Memory wise, perhaps yes; better not to use more memory than needed, especially if memcached is to share machine.

> And regarding memory,  
> if your blame output doesn't fit into your RAM, you have different kinds 
> of issues.

True.

> JFTR, I don't have any opinion about extending the porcelain output of 
> git-blame (apart from the fact that happens to not be useful for gitweb 
> for the reason I outlined in the previous paragraph).

It would be/might be (I haven't examined corner cases yet) important in the case of file history which both contains evil merges, and it's simplified history is different than full history.

-- 
Jakub Narebski
Poland