Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type

From: Jeff King <peff@peff.net>
Date: Apr 17, 2015, 14:23 UTC
Message-ID: <20150417142310.GA12479@peff.net>
In-Reply-To: <20150415221824.GB27566@peff.net>

On Wed, Apr 15, 2015 at 06:18:24PM -0400, Jeff King wrote:

Show 22 quoted lines

> > This _might_ have some performance impact in that strbuf_addch()
> > involves strbuf_grow(*, 1), which does "does it overflow to
> > increment sb->len by one?"; I would say it should be unmeasurable
> > because the function is expected to be used only on loose objects
> > and you shouldn't have very many of them without packing in your
> > repository in the first place.
> > 
> > I guess Peff's c1822d4f (strbuf: add an optimized 1-character
> > strbuf_grow, 2015-04-04) may want to teach strbuf_addch() to use his
> > new strbuf_grow_ch(), and once that happens the performance worry
> > would disappear without this code to be changed at all.
> 
> I haven't re-rolled that series yet, but the discussion there showed
> that strbuf_grow_ch() is unnecessary; call-sites can just check:
> 
>   if (!strbuf_avail(sb))
> 	strbuf_grow(sb, 1);
> 
> to get the fast inline check. Since we go to the trouble to inline
> strbuf_addch, we should probably teach it the same trick. It would be
> nice to identify a place with a tight strbuf_addch() loop that could
> demonstrate the speed increase, though.

Just for reference, I did teach strbuf_addch this trick. And the config code is the tight loop to test it with. Results are here:

  http://article.gmane.org/gmane.comp.version-control.git/267266

In this code path, we are typically only seeing short strings (the original code used a 10-byte static buffer). So I doubt it makes all that much difference.

If there _is_ a performance implication to worry about here, I think it would be that we are doing an extra malloc/free. I'm not sure I understand why we are copying it at all. The original code copied from the hdr into type[10] so that we could NUL-terminate it, which was required for type_from_string().

But now we use type_from_string_gently, which can accept a length[1]. So we could just count the bytes to the first space and pass the original buffer along with that length, no?

-Peff

[1] Not related to your patch, but it looks like type_from_string_gently
    is overly lax. It does a strncmp() with the length of the candidate
    name, but does not check that we consumed all of the matching name.
    So "tr" would match "tree", "comm" would match "commit", and so
    forth.