Re: [PATCH v2 2/2] sha1_file: improve sha1_file_name() perfs

From: Jeff King <peff@peff.net>
Date: Jan 17, 2018, 22:27 UTC
Message-ID: <20180117222707.GA16031@sigill.intra.peff.net>
In-Reply-To: <xmqq4lnkryt2.fsf@gitster.mtv.corp.google.com>

On Wed, Jan 17, 2018 at 12:54:33PM -0800, Junio C Hamano wrote:

Show 27 quoted lines

> Jeff Hostetler <git@jeffhostetler.com> writes:
> 
> >>     void sha1_file_name(struct strbuf *buf, const unsigned char
> >> *sha1)
> >>   {
> >> -	strbuf_addf(buf, "%s/", get_object_directory());
> >> +	const char *obj_dir = get_object_directory();
> >> +	size_t extra = strlen(obj_dir) + 1 + GIT_MAX_HEXSZ;
> >
> > Very minor nit.  Should this be "+3" rather than "+1"?
> > One for the slash after obj_dir, one for the slash between
> > "xx/y[38]", and one for the trailing NUL.
> >
> >>   +	if (extra > strbuf_avail(buf))
> >> +		strbuf_grow(buf, extra);
> 
> The callers who care use static strbuf with 1/2, which lets them
> grow it to an appropriate size after they make their first call.
> 
> On the other hand, the ones to which performance does not matter by
> definition do not care.
> 
> I actually think this whole "extra -> grow" business should be
> discarded.  With a miscomputed "extra" like this, it does not help
> anybody---everybody may pay cost for one extra realloc due to the
> miscalculation, and the ones that do care also do during their first
> call.

Let me second that. The diffstat here, along with the magic numbers, is not really encouraging unless we have a demonstrable speedup. In which case we can then measure and compare other approaches, like pushing a static strbuf farther up the stack. But without that, it feels like stumbling around in the dark.

-Peff