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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 25, 2008, 18:32 UTC
Message-ID: <7v3aika5l7.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20081025071912.GA24287@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

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

Ah, I missed that driver named "default".  Sorry for the noise.

Show 14 quoted lines

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

It would be either:

 (1) non-textconv users call fill_mmfile(&mf, ..., 0), use mf and return
     without clean-up as before, while textconv users do:

 	fill_mmfile(&mf, ..., 1);
        use mf;
        if (mf->ptr != one->data)
		clean up mf->ptr;
        return;

or

 (2) non-textconv users are unchanged from v1.6.0, while textconv users
     do:

	const char *textconv = get_textconv(...);
	fill_mmfile(&mf, ...); /* no change to fill_mmfile() */
	if (textconv)
        	munge_mmfile(&mf);
	use mf;
        if (textconv)
        	cleanup_mmfile(&mf);

The end result may not be that much different, but I find the latter easier to follow for three reasons:

 * we expect that majority of the users of fill_mmfile() are non textconv
   users.  I'd feel safer to keep their codepath the same as v1.6.0;

 * fill_mmfile() semantics is the same as long before -- it just gives it
   a borrowed pointer;

 * the code that makes a change to mmfile that requires clean-up does so
   explicitly, and that is followed by an explicit clean-up, both
   contained in the same function;

>   - performance considerations with driver loading. I believe this is a
>     non-issue. So either you are reading the code wrong,...

Yes I was.  Thanks and sorry.