Re: [PATCH] bisect: save heap memory. allocate only the required amount

[Ramsay Jones <ramsay@ramsay1.demon.co.uk>]

On 26/08/14 12:03, Jeff King wrote:
[snip]
> 
> Yeah, reading my suggestion again, it should clearly be
> alloc_flex_struct or something.
> 
> Here's a fully-converted sample spot, where I think there's a slight
> benefit:
> 
> diff --git a/remote.c b/remote.c
> index 3d6c86a..ba32d40 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -928,14 +928,30 @@ int remote_find_tracking(struct remote *remote, struct refspec *refspec)
>  	return query_refspecs(remote->fetch, remote->fetch_refspec_nr, refspec);
>  }
>  
> +static void *alloc_flex_struct(size_t base, size_t offset, const char *fmt, ...)
> +{
> +	va_list ap;
> +	size_t extra;
> +	char *ret;
> +
> +	va_start(ap, fmt);
> +	extra = vsnprintf(NULL, 0, fmt, ap);
> +	extra++; /* for NUL terminator */
> +	va_end(ap);
> +
> +	ret = xcalloc(1, base + extra);
> +	va_start(ap, fmt);
> +	vsnprintf(ret + offset, extra, fmt, ap);

What is the relationship between 'base' and 'offset'?

Let me assume that base is always, depending on your compiler, either
equal to offset or offset+1. Yes? (I'm assuming base is always the
sizeof(struct whatever)). Do you need both base and offset?

> +	va_end(ap);
> +
> +	return ret;
> +}
> +
>  static struct ref *alloc_ref_with_prefix(const char *prefix, size_t prefixlen,
>  		const char *name)
>  {
> -	size_t len = strlen(name);
> -	struct ref *ref = xcalloc(1, sizeof(struct ref) + prefixlen + len + 1);
> -	memcpy(ref->name, prefix, prefixlen);
> -	memcpy(ref->name + prefixlen, name, len);
> -	return ref;
> +	return alloc_flex_struct(sizeof(struct ref), offsetof(struct ref, name),
> +				 "%.*s%s", prefixlen, prefix, name);
>  }
>  
>  struct ref *alloc_ref(const char *name)
> 
> Obviously the helper is much longer than the code it is replacing, but
> it would be used in multiple spots. The main thing I like here is that
> we are dropping the manual length computations, which are easy to get
> wrong (it's easy to forget a +1 for a NUL terminator, etc).
> 
> The offsetof is a little ugly. And the fact that we have a pre-computed
> length for prefixlen makes the format string a little ugly.
> 
> Here's a another example:
> 
> diff --git a/attr.c b/attr.c
> index 734222d..100c423 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -89,8 +89,8 @@ static struct git_attr *git_attr_internal(const char *name, int len)
>  	if (invalid_attr_name(name, len))
>  		return NULL;
>  
> -	a = xmalloc(sizeof(*a) + len + 1);
> -	memcpy(a->name, name, len);
> +	a = alloc_flex_array(sizeof(*a), offsetof(struct git_attr, name),
> +			     "%.*s", len, name);
>  	a->name[len] = 0;
>  	a->h = hval;
>  	a->next = git_attr_hash[pos];
> 
> I think this is strictly worse for reading. The original computation was
> pretty easy in the first place, so we are not getting much benefit
> there. And again we have the precomputed "len" passed into the function,
> so we have to use the less readable "%.*s". And note that offsetof()
> requires us to pass a real typename instead of just "*a", as sizeof()
> allows (I suspect passing "a->name - a" would work on many systems, but
> I also suspect that is a gross violation of the C standard when "a" has
> not yet been initialized).
> 
> So given that the benefit ranges from "a little" to "negative" in these
> two examples, I'm inclined to give up this line of inquiry.

Indeed. :-D

ATB,
Ramsay Jones