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

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

William Duclot <william.duclot@ensimag.grenoble-inp.fr> writes:

> multiple threads. Those limitations prevent strbuf to be used in

prevent strbuf from being used ...

> API ENHANCEMENT
> ---------------
>
> All functions of the API can still be reliably called without
> knowledge of the initialization (normal/preallocated/fixed) with the
> exception that strbuf_grow() may die() if the string try to overflow a

s/try/tries/

> @@ -20,16 +28,37 @@ char strbuf_slopbuf[1];
>  
>  void strbuf_init(struct strbuf *sb, size_t hint)
>  {
> +	sb->flags = 0;
>  	sb->alloc = sb->len = 0;
>  	sb->buf = strbuf_slopbuf;
>  	if (hint)
>  		strbuf_grow(sb, hint);
>  }

If you set flags = 0 here, existing callers will have all flags off,
including OWNS_MEMORY.

I *think* this is OK, as sb->buf is currently pointing to
strbuf_slopbuf, which the the strbuf doesn't own. But that is too subtle
to go without an explanatory comment IMHO.

Also, doesn't this make the "new_buf" case useless in strbuf_grow?

With your patch, the code looks like:

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.

> +void strbuf_wrap_preallocated(struct strbuf *sb, char *path_buf,
> +			      size_t path_buf_len, size_t alloc_len)
> +{
> +	if (!path_buf)
> +		die("you try to use a NULL buffer to initialize a strbuf");
> +
> +	strbuf_init(sb, 0);
> +	strbuf_attach(sb, path_buf, path_buf_len, alloc_len);
> +	sb->flags &= ~STRBUF_OWNS_MEMORY;
> +	sb->flags &= ~STRBUF_FIXED_MEMORY;
> +}

strbuf_wrap_preallocated seem very close to strbuf_attach. I'd rather
see a symmetric code sharing like

void strbuf_attach_internal(struct strbuf *sb, ..., unsigned int flags)

and then strbuf_attach() and strbuf_wrap_preallocated() become
straightforward wrappers around it.

This would avoid setting and then unsetting STRBUF_OWNS_MEMORY (the
performance impact is probably small, but it looks weird to set the flag
and then unset it right away).

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.

* strbuf_attach() calls strbuf_release(), which allows reusing an
  existing strbuf. strbuf_wrap_preallocated() calls strbuf_init which
  would override silently any previous content. I think strbuf_attach()
  does the right thing here.

  (And I'm probably the one who misguided you to do this)

  In any case, you probably want to include calls to strbuf_attach() and
  strbuf_wrap_*() functions on existing non-empty strbufs.

> +void strbuf_wrap_fixed(struct strbuf *sb, char *path_buf,
> +		       size_t path_buf_len, size_t alloc_len)
> +{
> +	strbuf_wrap_preallocated(sb, path_buf, path_buf_len, alloc_len);
> +	sb->flags |= STRBUF_FIXED_MEMORY;
> +}

And this could become a 3rd caller of strbuf_attach_internal().

> @@ -61,9 +96,32 @@ void strbuf_grow(struct strbuf *sb, size_t extra)
>  	if (unsigned_add_overflows(extra, 1) ||
>  	    unsigned_add_overflows(sb->len, extra + 1))
>  		die("you want to use way too much memory");
> -	if (new_buf)
> -		sb->buf = NULL;
> -	ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
> +	if ((sb->flags & STRBUF_FIXED_MEMORY) && sb->len + extra + 1 > sb->alloc)
> +		die("you try to make a string overflow the buffer of a fixed strbuf");
> +
> +	/*
> +	 * ALLOC_GROW may do a realloc() if needed, so we must not use it on
> +	 * a buffer the strbuf doesn't own
> +	 */
> +	if (sb->flags & STRBUF_OWNS_MEMORY) {
> +		if (new_buf)
> +			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);

I think you want to memcpy only sb->len + 1 bytes. Here, you're
memcpy-ing the allocated-but-not-initialized part of the array.

xmemdupz can probably simplify the code too (either you include the '\0'
in what memcpy copies, or you let xmemdupz add it).

> +/**
> + * Allow the caller to give a pre-allocated piece of memory for the strbuf
> + * to use. It is possible then to strbuf_grow() the string past the size of the
> + * pre-allocated buffer: a new buffer will be allocated. The pre-allocated

To make it clearer: "a new buffer will then be allocated"?

> +/**
> + * 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?

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

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/