From: Jeff King <peff@peff.net>
Date: Wed, 15 Apr 2015 22:18:24 GMT
Subject: Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type
Message-ID: <20150415221824.GB27566@peff.net>
In-Reply-To: <xmqqmw29jg78.fsf@gitster.dls.corp.google.com>

On Wed, Apr 15, 2015 at 01:21:15PM -0700, Junio C Hamano wrote:

> > -		type[i++] = c;
> > -		if (i >= sizeof(type))
> > -			return -1;
> > +		strbuf_addch(&typename, c);
> >  	}
> > -	type[i] = 0;
> 
> 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.

-Peff