From: Matthieu Moy <Matthieu.Moy@grenoble-inp•fr>
To: William Duclot <william.duclot@ensimag•grenoble-inp.fr>
Cc: git@vger•kernel.org,
simon rabourg <simon.rabourg@ensimag•grenoble-inp.fr>,
francois beutin <francois.beutin@ensimag•grenoble-inp.fr>,
antoine queru <antoine.queru@ensimag•grenoble-inp.fr>,
mhagger@alum•mit.edu
Subject: Re: [PATCH 2/2] strbuf: allow to use preallocated memory
Date: Mon, 30 May 2016 16:34:43 +0200 [thread overview]
Message-ID: <vpqpos38vi4.fsf@anie.imag.fr> (raw)
In-Reply-To: <1639412597.204503.1464617754937.JavaMail.zimbra@ensimag.grenoble-inp.fr> (William Duclot's message of "Mon, 30 May 2016 16:15:54 +0200 (CEST)")
William Duclot <william.duclot@ensimag•grenoble-inp.fr> writes:
> Matthieu Moy <Matthieu.Moy@grenoble-inp•fr> writes:
>
>> void strbuf_grow(struct strbuf *sb, size_t extra)
>> {
>> int new_buf = !sb->alloc;
>> ...
>> if (sb->flags & STRBUF_OWNS_MEMORY) {
>> if (new_buf) // <---------------------------------------- (1)
>> sb->buf = NULL;
>> ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
>> } else {
>> /*
>> * The strbuf doesn't own the buffer: to avoid to realloc it,
>> * the strbuf needs to use a new buffer without freeing the old
>> */
>> if (sb->len + extra + 1 > sb->alloc) {
>> size_t new_alloc = MAX(sb->len + extra + 1, alloc_nr(sb->alloc));
>> char *buf = xmalloc(new_alloc);
>> memcpy(buf, sb->buf, sb->alloc);
>> sb->buf = buf;
>> sb->alloc = new_alloc;
>> sb->flags |= STRBUF_OWNS_MEMORY;
>> }
>> }
>>
>> if (new_buf) // <---------------------------------------- (2)
>> sb->buf[0] = '\0';
>> }
>>
>> I think (1) is now dead code, since sb->alloc == 0 implies that
>> STRBUF_OWNS_MEMORY is set. (2) seems redundant since you've just
>> memcpy-ed the existing '\0' into the buffer.
>
> You're right for (1), I hadn't noticed that.
> For (2), we'll still have to set sb->buf[new_alloc-1]='\0' after the memcpy, if we
> have sb->alloc==0 then the memcpy won't copy it.
That sounds like one more reason to memcpy len + 1 bytes, and you'll get
the '\0' copied.
>> After your patch, there are differences between
>> strbuf_wrap_preallocated() which I think are inconsistencies:
>>
>> * strbuf_attach() does not check for NULL buffer, but it doesn't accept
>> them either if I read correctly. It would make sense to add the check
>> to strbuf_attach(), but it's weird to have the performance-critical
>> oriented function do the expensive stuff that the
>> non-performance-critical one doesn't.
>
> I agree that strbuf_attach should do the check (it seems strange that it
> doesn't already do it, as the "buffer never NULL" invariant is not new).
> I don't understand your "but" part, what "expensive stuff" are you talking
> about?
"expensive stuff" was an exageration for "== NULL" test. It's not that
expensive, but costs a tiny bit of CPU time.
> xmemdupz can only allocate the same size it will copy.
Indeed, so forget about it.
>>> +/**
>>> + * Allow the caller to give a pre-allocated piece of memory for the strbuf
>>> + * to use and indicate that the strbuf must use exclusively this buffer,
>>> + * never realloc() it or allocate a new one. It means that the string can
>>> + * be manipulated but cannot overflow the pre-allocated buffer. The
>>> + * pre-allocated buffer will never be freed.
>>> + */
>>
>> Perhaps say explicitly that although the allocated buffer has a fixed
>> size, the string itself can grow as long as it does not overflow the
>> buffer?
>
> That's what I meant by "the string can be manipulated but cannot overflow
> the pre-allocated buffer". I'll try to reformulate
Maybe "the string can grow, but cannot overflow"?
>>> @@ -91,6 +116,8 @@ extern void strbuf_release(struct strbuf *);
>>> * Detach the string from the strbuf and returns it; you now own the
>>> * storage the string occupies and it is your responsibility from then on
>>> * to release it with `free(3)` when you are done with it.
>>> + * Must allocate a copy of the buffer in case of a preallocated/fixed
>>> buffer.
>>> + * Performance-critical operations have to be aware of this.
>>
>> Better than just warn about performance, you can give the alternative.
>
> I'm not sure what you mean, I don't think there really is an alternative for
> detaching a string?
So, is the comment above saying "You're doomed, there's no way you can
get good performance anyway"?
The alternative is just that you don't have to call strbuf_release since
the caller can access the .buf field and is already the one responsible
for freeing it when needed, and it's safe to just call strbuf_init() if
one needs to re-initialize the stbuf structure.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
next prev parent reply other threads:[~2016-05-30 14:34 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-30 10:36 [PATCH 0/2] strbuf: improve API William Duclot
2016-05-30 10:36 ` [PATCH 1/2] strbuf: add tests William Duclot
2016-05-30 11:26 ` Johannes Schindelin
2016-05-30 13:42 ` Simon Rabourg
2016-05-30 11:56 ` Matthieu Moy
2016-05-31 2:04 ` Michael Haggerty
2016-05-31 9:48 ` Simon Rabourg
2016-05-30 10:36 ` [PATCH 2/2] strbuf: allow to use preallocated memory William Duclot
2016-05-30 12:13 ` Johannes Schindelin
2016-05-30 13:20 ` William Duclot
2016-05-31 6:21 ` Johannes Schindelin
2016-05-31 3:05 ` Michael Haggerty
2016-05-31 6:41 ` Johannes Schindelin
2016-05-31 8:25 ` Michael Haggerty
2016-05-30 12:52 ` Matthieu Moy
2016-05-30 14:15 ` William Duclot
2016-05-30 14:34 ` Matthieu Moy [this message]
2016-05-30 15:16 ` William Duclot
2016-05-31 4:05 ` Michael Haggerty
2016-05-31 15:59 ` William Duclot
2016-06-03 14:04 ` William Duclot
2016-05-30 21:56 ` Mike Hommey
2016-05-30 22:46 ` William Duclot
2016-05-30 22:50 ` Mike Hommey
2016-05-31 6:34 ` Junio C Hamano
2016-05-31 15:45 ` William
2016-05-31 15:54 ` Matthieu Moy
2016-05-31 16:08 ` William Duclot
2016-05-30 11:32 ` [PATCH 0/2] strbuf: improve API Remi Galan Alfonso
2016-06-01 7:42 ` Jeff King
2016-06-01 19:50 ` David Turner
2016-06-01 20:09 ` Jeff King
2016-06-01 20:22 ` David Turner
2016-06-01 21:07 ` Jeff King
2016-06-02 11:11 ` Michael Haggerty
2016-06-02 12:58 ` Matthieu Moy
2016-06-02 14:22 ` William Duclot
2016-06-24 17:20 ` Jeff King
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=vpqpos38vi4.fsf@anie.imag.fr \
--to=matthieu.moy@grenoble-inp$(echo .)fr \
--cc=antoine.queru@ensimag$(echo .)grenoble-inp.fr \
--cc=francois.beutin@ensimag$(echo .)grenoble-inp.fr \
--cc=git@vger$(echo .)kernel.org \
--cc=mhagger@alum$(echo .)mit.edu \
--cc=simon.rabourg@ensimag$(echo .)grenoble-inp.fr \
--cc=william.duclot@ensimag$(echo .)grenoble-inp.fr \
/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