Re: [PATCH (GITK) v2 4/4] gitk: Implement batch lookup and caching of encoding attrs.

From: Alexander Gavrilov <angavrilov@gmail.com>
Date: Oct 11, 2008, 09:28 UTC
Message-ID: <200810111328.50951.angavrilov@gmail.com>
In-Reply-To: <18671.62417.328489.317909@cargo.ozlabs.ibm.com>

On Saturday 11 October 2008 04:31:13 Paul Mackerras wrote:

Show 13 quoted lines

> > > Also, I wonder why we now have two levels of caching of the encoding
> > > attribute.  Your patch 1/4 introduced path_encoding_cache, which was
> > > fine, but now we have path_attr_cache as well, which seems to me to
> > > serve exactly the same function since the encoding is the only
> > > attribute we ever ask about.  Surely we don't need both caches?
> > 
> > If the (git-gui) patch that reimplements the tcl_encoding procedure is
> > applied, we may drop the path_encoding_cache. Current implementation
> > is too slow for batch lookup, especially if the encoding is actually
> > not supported, and without the cache the lookup would be done on every
> > loading of a diff.
> 
> I was thinking more in terms of dropping the path_attr_cache actually.

Since gitattr is a general-purpose function that can read any attribute, I decided that it should use its own cache.

Basically, all this double-caching issue is fallout from my failure to anticipate the need of batch attribute lookup from the beginning...

Show 11 quoted lines

> Actually, if [tcl_encoding] is slow, then why is $gui_encoding the
> untranslated version, so that we do [tcl_encoding $gui_encoding] on
> each call to get_path_encoding?  Why don't we do the tcl_encoding call
> once and have $gui_encoding be the result of that?  In fact
> $gui_encoding should be the result of this code (from
> get_path_encoding):
> 
> 	set tcl_enc [tcl_encoding $gui_encoding]
> 	if {$tcl_enc eq {}} {
> 		set tcl_enc [encoding system]
> 	}

Well, that code was copied from git-gui, where it looks like this:

set tcl_enc [tcl_encoding [get_config gui.encoding]]

I.e. there is no "$gui_encoding" variable, although get_config does use a cache.

> And if [tcl_encoding] is slow, then it should have a cache.  There's
> only likely to be at most 2 or 3 values it gets called for, and it's
> a constant function.

In git-gui the slowdown appeared during the construction of the menu listing all available encodings, so a simple cache would not have helped. I reimplemented it using a lookup table to resolve aliases (constructed on the first run). But it can be thought of as a precalculated cache.

> At this point, what I think I might do is apply your set of patches
> (but with 2/4 and 3/4 folded into a single patch) and then go through
> and do another commit that addresses the concerns I've raised.  OK?

Maybe I should resend the patches, scrapping path_encoding_cache, and adding the optimized version of tcl_encoding?

Alexander