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

From: Paul Mackerras <paulus@samba.org>
Date: Oct 11, 2008, 00:31 UTC
Message-ID: <18671.62417.328489.317909@cargo.ozlabs.ibm.com>
In-Reply-To: <bb6f213e0810100522v653507d6r75cc4c64b57aa459@mail.gmail.com>

Alexander Gavrilov writes:

> OS-enforced command-line size limit on Windows is 32K. Cramming in
> 1000 paths would leave only 32 characters for each path.

Eeek.  OK.

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

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]
	}

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.

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?

Paul.