Jeff King <peff@xxxxxxxx> writes:
>> 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.
> 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.
--
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
