From: Ramsay Jones <ramsay@ramsay1•demon.co.uk>
To: Jeff King <peff@peff•net>, Junio C Hamano <gitster@pobox•com>
Cc: "Stefan Beller" <stefanbeller@gmail•com>,
"Arjun Sreedharan" <arjun024@gmail•com>,
"Git Mailing List" <git@vger•kernel.org>,
"Christian Couder" <chriscool@tuxfamily•org>,
"Nguyễn Thái Ngọc Duy" <pclouds@gmail•com>
Subject: Re: [PATCH] bisect: save heap memory. allocate only the required amount
Date: Tue, 26 Aug 2014 12:57:21 +0100 [thread overview]
Message-ID: <53FC7621.7090102@ramsay1.demon.co.uk> (raw)
In-Reply-To: <20140826110303.GA25736@peff.net>
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
next prev parent reply other threads:[~2014-08-26 11:57 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-08-24 14:17 [PATCH] bisect: save heap memory. allocate only the required amount Arjun Sreedharan
2014-08-24 15:10 ` Stefan Beller
2014-08-24 23:39 ` Junio C Hamano
2014-08-25 13:07 ` Jeff King
2014-08-25 21:36 ` Junio C Hamano
2014-08-26 11:03 ` Jeff King
2014-08-26 11:57 ` Ramsay Jones [this message]
2014-08-26 12:14 ` Jeff King
2014-08-26 12:37 ` Ramsay Jones
2014-08-26 12:43 ` Jeff King
2014-08-26 12:59 ` Ramsay Jones
2014-08-24 15:32 ` Ramsay Jones
2014-08-24 21:55 ` Arjun Sreedharan
2014-08-25 13:35 ` Jeff King
2014-08-25 14:06 ` Christian Couder
2014-08-25 15:00 ` Jeff King
2014-08-25 18:26 ` Junio C Hamano
2014-08-25 19:35 ` Jeff King
2014-08-25 20:27 ` Arjun Sreedharan
2014-08-25 21:11 ` Junio C Hamano
2014-08-26 10:20 ` [PATCH 0/3] name_decoration cleanups Jeff King
2014-08-26 10:23 ` [PATCH 1/3] log-tree: make add_name_decoration a public function Jeff King
2014-08-26 11:48 ` Ramsay Jones
2014-08-26 12:17 ` Jeff King
2014-08-26 10:23 ` [PATCH 2/3] log-tree: make name_decoration hash static Jeff King
2014-08-26 17:40 ` Junio C Hamano
2014-08-26 17:43 ` Jeff King
2014-08-26 19:25 ` Junio C Hamano
2014-08-26 10:24 ` [PATCH 3/3] log-tree: use FLEX_ARRAY in name_decoration Jeff King
2014-08-27 0:30 ` Eric Sunshine
2014-08-25 21:14 ` [PATCH] bisect: save heap memory. allocate only the required amount Junio C Hamano
2014-08-26 7:30 ` Arjun Sreedharan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53FC7621.7090102@ramsay1.demon.co.uk \
--to=ramsay@ramsay1$(echo .)demon.co.uk \
--cc=arjun024@gmail$(echo .)com \
--cc=chriscool@tuxfamily$(echo .)org \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=pclouds@gmail$(echo .)com \
--cc=peff@peff$(echo .)net \
--cc=stefanbeller@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox