Re: [PATCH 4/7] textconv: don't convert for every operation

From: Jeff King <peff@peff.net>
Date: Oct 25, 2008, 07:19 UTC
Message-ID: <20081025071912.GA24287@coredump.intra.peff.net>
In-Reply-To: <7vhc71b5ai.fsf@gitster.siamese.dyndns.org>

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.

Show 5 quoted lines

> 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