On Fri, Oct 24, 2008 at 10:41:09PM -0700, Junio C Hamano wrote:
> Isn't this function "fill_mmfile()" and its callers leaky as a sieve?
Yes, it is (for the textconv case; the other is unaffected). That was
another reason I had attached the data to the diff_filespec, but
obviously I forgot about that when re-rolling the patch.
> I am also somewhat worried about the performance impact of running
> get_textconv() to the same filespec many times when no textconv is
> defined, which is the normal case we should optimize for. It appears that
> diff_filespec_load_driver() is optimized for a wrong case (i.e. "we
> already know this needs a custom driver and we know which one").
No, it is the same as before. We always end up with a driver at the end
of the function, so further calls will be no-ops. So we do exactly one
attribute lookup per filespec, caching the result.
> I am inclined to suggest reverting the whole series (including the ones
> that are already in 'master') and start over from scratch, limiting the
> run_textconv() to only inside diff.c::builtin_diff() (in the else {} block
> where "Crazy xcl interfaces.." comment appears).
I am not terribly opposed to that (I was quite surprised to see the
original make it to 'next', let alone 'master'). OTOH, the real reason
to do so would be to keep a clean history, and it is already too late
for that.
I think it makes sense to figure out _what_ needs fixed first, because
it might be somewhat minor. So far I see:
- leak from fill_mmfile; this definitely needs fixed. The quick fix is
minor (free if we did a textconv). A more involved fix is to pull it
out of fill_mmfile entirely and put the code directly into
builtin_diff, which would be part of a re-roll of this latest
series. But see below.
- Keep fill_mmfile allocation semantics clear. I was trying to keep
it simple for other fill_mmfile callers to opt-in to textconv, even
if they chose to do it by some user-controlled mechanism instead of
by default (e.g., diff.TextconvDiffstat or something). But maybe
that is not of value to us. Again, that is a re-roll of this series.
- performance considerations with driver loading. I believe this is a
non-issue. So either you are reading the code wrong, or I am not
understanding your concern correctly (or _I_ am reading the code
wrong, of course).
Others?
-Peff
--
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
