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

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

quoted

quoted

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...

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