Re: [PATCH 2/2] strbuf: allow to use preallocated memory

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 31, 2016, 06:41 UTC
Message-ID: <alpine.DEB.2.20.1605310822490.4449@virtualbox>
In-Reply-To: <574CFF75.3090805@alum.mit.edu>

Hi Michael,

On Tue, 31 May 2016, Michael Haggerty wrote:

Show 29 quoted lines

> On 05/30/2016 02:13 PM, Johannes Schindelin wrote:
> > [...]
> >> @@ -38,7 +67,11 @@ char *strbuf_detach(struct strbuf *sb, size_t *sz)
> >>  {
> >>  	char *res;
> >>  	strbuf_grow(sb, 0);
> >> -	res = sb->buf;
> >> +	if (sb->flags & STRBUF_OWNS_MEMORY)
> >> +		res = sb->buf;
> >> +	else
> >> +		res = xmemdupz(sb->buf, sb->alloc - 1);
> > 
> > This looks like a usage to be avoided: if we plan to detach the buffer,
> > anyway, there is no good reason to allocate it on the heap first. I would
> > at least issue a warning here.
> 
> I think this last case should be changed to
> 
>     res = xmemdupz(sb->buf, sb->len);
> 
> Johannes, if this change is made then I think that there is a reasonable
> use case for calling `strbuf_detach()` on a strbuf that wraps a
> stack-allocated string, so I don't think that a warning is needed.
> 
> I think this change makes sense. After all, once a caller detaches a
> string, it is probably not planning on growing/shrinking it anymore, so
> any more space than that would probably be wasted. In fact, since the
> caller has no way to ask how much extra space the detached string has
> allocated, it is almost guaranteed that the space would be wasted.

Ah, I did not think of that. (We could of course introduce strbuf_realloc_snugly() or some such, but I agree: why?) So the best counter-example to my objection might be something like this:

-- snip --
Subject: git_pathdup(): avoid realloc()

When generating Git paths, we already know the maximum length: MAX_PATH. Let's avoid realloc()-caused fragmentation by using a fixed buffer.

As a side effect, we avoid overallocating the end result, too: previously, strbuf_detach() would not realloc() to avoid wasting the (sb->alloc - sb->len) bytes, while it malloc()s the precisely needed amount of memory when detaching fixed buffers.

diff --git a/path.c b/path.c
index 2511d8a..64fd3ee 100644
--- a/path.c
+++ b/path.c
@@ -426,7 +426,8 @@ const char *git_path(const char *fmt, ...)
 
 char *git_pathdup(const char *fmt, ...)
 {
-	struct strbuf path = STRBUF_INIT;
+	char buffer[MAX_PATH];
+	struct strbuf path = STRBUF_WRAP_FIXED(buffer);
 	va_list args;
 	va_start(args, fmt);
 	do_git_path(&path, fmt, args);
-- snap --

Color me convinced,
Dscho