Re: [PATCH] Optimize common pattern of alloc_ref from string

From: Jeff King <peff@peff.net>
Date: May 11, 2008, 08:07 UTC
Message-ID: <20080511080709.GA6971@sigill.intra.peff.net>
In-Reply-To: <7ce338ad0805101730n5b964a0em39d9fdcd9fc45f00@mail.gmail.com>

On Sat, May 10, 2008 at 05:30:56PM -0700, Krzysztof Kowalczyk wrote:

Show 8 quoted lines

> >> -     ref = alloc_ref(strlen(refname) + 1);
> >> -     strcpy(ref->name, refname);
> >> +     ref = alloc_ref_from_str(refname);
> >
> > So this turns a 2-line construct into a 1-line construct...
> 
> And avoids future prossible mistakes with not terminating the string,
> like the one just commited.

Yes. Please don't interpret my comment as "this change isn't worth it"; I meant it as "yes, it is good to be simplifying and making this part less error prone; I just want to nitpick your exact implementation".

> You're absolutely right - it's a micro-optimization and your version
> might be preferred for clarity. This is the first time I submit a
> patch to git so I don't have a good feel for what kind of treadoffs
> people find acceptable.

OK. Ultimately it is up to Junio. I think your version is a bit complex, but at the very least it contains that complexity neatly in one function.

Actually, the version I posted actually has an optimization, too (it remembers the strlen calculation to reuse it). I think the simplest would just be:

  struct ref *alloc_ref_from_str(cons char *str)
  {
    struct ref *ret = alloc_ref(strlen(str) + 1);
    strcpy(ret->name, str);
    return ret;
  }

But really my main worry is that now we have _two_ functions which allocate refs, so if "struct ref" ever grows a new field that needs initializing, it has to go in two places (whereas if alloc_ref_from_str calls alloc_ref, it works automatically).

Show 18 quoted lines

> I should also mention that
> static struct ref *try_explicit_object_name(const char *name)
> {
> 	unsigned char sha1[20];
> 	struct ref *ref;
> 
> 	if (!*name) {
> 		ref = alloc_ref(20);
> 		strcpy(ref->name, "(delete)");
> 		hashclr(ref->new_sha1);
> 		return ref;
> 	}
> ...
> 
> could also be replaced with alloc_ref_str() - I just wasn't 100% sure
> if overallocating 10 bytes (20 - strlen("(delete)")) was just sloppy
> code or does other code relies on that (which is unlikely and if true
> then it wouldn't be good).

It looks like just slop, and should probably be fixed at the same time. There are also quite a number of:

  ret = alloc_ref(strlen(name) + 6);
  sprintf(ret->name, "refs/%s", name);

which are error-prone and owuld be nice to fix. However, I don't think there is an easy way short of making ref->name a strbuf. And now it seems we are getting into a lot of code churn for a relatively small benefit.

-Peff