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

From: Krzysztof Kowalczyk <kkowalczyk@gmail.com>
Date: May 11, 2008, 00:30 UTC
Message-ID: <7ce338ad0805101730n5b964a0em39d9fdcd9fc45f00@mail.gmail.com>
In-Reply-To: <20080510233918.GA315@sigill.intra.peff.net>

On Sat, May 10, 2008 at 4:39 PM, Jeff King <peff@peff.net> wrote:

Show 8 quoted lines

> On Sat, May 10, 2008 at 04:26:58PM -0700, kkowalczyk@gmail.com wrote:
>
>> As a byproduct, fixes one place where string wasn't properly terminated.
>
> Great. Does this fix a user-visible bug? It would be nice to mention in
> the commit log _which_ place (though after reading the patch carefully,
> it looks like the one interpret_target) so that people looking at the
> commit later can understand exactly what was fixed.

It was a subtle memory corruption that wouldn't cause problems in 99.99% cases, but valgrind would probably catch it. And yes, it's the interp_target().

Show 5 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.

Show 27 quoted lines

>> +struct ref *alloc_ref_from_str(const char* str)
>> +{
>> +     struct ref *ret;
>> +     unsigned len = strlen(str) + 1;
>> +     char *tmp = xmalloc(sizeof(struct ref) + len);
>> +     ret = (struct ref*)tmp;
>> +     memset(tmp, 0, sizeof(struct ref));
>> +     tmp += sizeof(struct ref);
>> +     memcpy(tmp, str, len);
>> +     return ret;
>> +}
>
> But why do we need an 8-line function to do it?
>
> The only difference I can see over
>
>  struct ref *alloc_ref_from_str(const char *str)
>  {
>    unsigned len = strlen(str) + 1;
>    struct ref *ret = alloc_ref(len);
>    memcpy(ret->name, str, len);
>    return ret;
>  }
>
> is that we avoid memsetting the name portion of the struct to 0 before
> copying to it. It seems like an unproven micro-optimization that makes
> it a bit harder to read.

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.

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).

Regards,

-- kjk