On Saturday 11 October 2008 04:31:13 Paul Mackerras wrote:
> > > 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
