git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
WDWilliam Duclot <william.duclot@ensimag.grenoble-inp.fr>
Date
May 30, 2016, 13:20 UTC
Message-ID
<953965621.202433.1464614453377.JavaMail.zimbra@ensimag.grenoble-inp.fr>
In-Reply-To
<alpine.DEB.2.20.1605301326530.4449@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 43 quoted lines
> On Mon, 30 May 2016, William Duclot wrote:
> 
>> It is unfortunate that it is currently impossible to use a strbuf
>> without doing a memory allocation. So code like
>> 
>> void f()
>> {
>>     char path[PATH_MAX];
>>     ...
>> }
>> 
>> typically gets turned into either
>> 
>> void f()
>> {
>>     struct strbuf path;
>>     strbuf_add(&path, ...); <-- does a malloc
>>     ...
>>     strbuf_release(&path);  <-- does a free
>> }
>> 
>> which costs extra memory allocations, or
>> 
>> void f()
>> {
>>     static struct strbuf path;
>>     strbuf_add(&path, ...);
>>     ...
>>     strbuf_setlen(&path, 0);
>> }
>> 
>> which, by using a static variable, avoids most of the malloc/free
>> overhead, but makes the function unsafe to use recursively or from
>> multiple threads. Those limitations prevent strbuf to be used in
>> performance-critical operations.
> 
> This description is nice and verbose, but maybe something like this would
> introduce the subject in a quicker manner?
> 
> 	When working e.g. with file paths or with dates, strbuf's
> 	malloc()/free() dance of strbufs can be easily avoided: as
> 	a sensible initial buffer size is already known, it can be
> 	allocated on the heap.

strbuf already allow to indicate a sensible initial buffer size thanks to strbuf_init() second parameter. The main perk of pre-allocation is to use stack-allocated memory, and not heap-allocated :) Unless I misunderstood your message?

Show 11 quoted lines
>> diff --git a/strbuf.c b/strbuf.c
>> index 1ba600b..527b986 100644
>> --- a/strbuf.c
>> +++ b/strbuf.c
>> @@ -1,6 +1,14 @@
>>  #include "cache.h"
>>  #include "refs.h"
>>  #include "utf8.h"
>> +#include <sys/param.h>
> 
> Why?
For the MAX macro. It may be a teeny tiny overkill
Show 9 quoted lines
>> +/**
>> + * Flags
>> + * --------------
>> + */
>> +#define STRBUF_OWNS_MEMORY 1
>> +#define STRBUF_FIXED_MEMORY (1 << 1)
> 
> From reading the commit message, I expected STRBUF_OWNS_MEMORY.
> STRBUF_FIXED_MEMORY still needs to be explained.
Yes, that seems right
Show 23 quoted lines
>> @@ -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);
>>  }
>>  
>> +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;
> 
> Shorter: sb->flags &= ~(STRBUF_OWNS_MEMORY | STRBUF_FIXED_MEMORY);
Okay with me
Show 13 quoted lines
>> +}
>> +
>> +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;
>> +}
> 
> Rather than letting strbuf_wrap_preallocated() set sb->flags &=
> ~FIXED_MEMORY only to revert that decision right away, a static function
> could be called by both strbuf_wrap_preallocated() and
> strbuf_wrap_fixed().
Makes sense
Show 10 quoted lines
>>  void strbuf_release(struct strbuf *sb)
>>  {
>>  	if (sb->alloc) {
>> -		free(sb->buf);
>> +		if (sb->flags & STRBUF_OWNS_MEMORY)
>> +			free(sb->buf);
>>  		strbuf_init(sb, 0);
>>  	}
> 
> Should we not reset the flags here, too?

Well, strbuf_init() reset the flags. The only way to have !sb->alloc is that strbuf has been initialized and never used (even alloc_grow(0) set sb->alloc=1), so sb==STRBUF_INIT, so the flags don't have to be reset

Show 13 quoted lines
>> @@ -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.
strbuf_detach() guarantees to return heap-allocated memory, that the caller
can use however he want and that he'll have to free. If the strbuf doesn't
own the memory, it cannot return the buf attribute directly because:
- The memory belong to someone else (so the caller can't use it however
he want)
- The caller can't have the responsibility to free (because the memory
belong to someone else)
- The memory may not even be heap-allocated
Show 22 quoted lines
>> @@ -51,6 +84,8 @@ void strbuf_attach(struct strbuf *sb, void *buf, size_t
>> len, size_t alloc)
>>  	sb->buf   = buf;
>>  	sb->len   = len;
>>  	sb->alloc = alloc;
>> +	sb->flags |= STRBUF_OWNS_MEMORY;
>> +	sb->flags &= ~STRBUF_FIXED_MEMORY;
>>  	strbuf_grow(sb, 0);
>>  	sb->buf[sb->len] = '\0';
>>  }
>> @@ -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");
> 
> We try to avoid running over 80 columns/row. This message could be
> more to the point: cannot grow fixed string
What is fixed is the buffer, not the string. I'll shrink that under 80 columns
Show 6 quoted lines
>>  extern char strbuf_slopbuf[];
>> -#define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
>> +#define STRBUF_INIT  { 0, 0, 0, strbuf_slopbuf }
> 
> If I am not mistaken, to preserve the existing behavior the initial flags
> should be 1 (own memory).

strbuf_slopbuf is a buffer that doesn't belong to any strbuf (because it's shared between all just-initialized strbufs). If STRBUF_OWNS_MEMORY was set, strbuf_slopbuf could be freed (which is impossible because it is shared AND even more because it is stack-allocated) 

> BTW this demonstrates that it may not be a good idea to declare the
> "flags" field globally but then make the actual flags private.
I'm not sure what you mean here?
> Also: similar use cases in Git used :1 flags (see e.g. the "configured"
> field in credential.h).

I think that keeping an obscure `flags` attribute may be better, as they should only be useful for internal operations and the user shouldn't mess with it. Keeping it a `private` attribute, in a way

Previous: Johannes SchindelinNext: Johannes Schindelin
Message 10 of 38 in “strbuf: improve API”
  1. 0/2 strbuf: improve APIWilliam Duclot, May 30, 2016
  2. 1/2 strbuf: add testsWilliam Duclot, May 30, 2016
  3. Johannes SchindelinMay 30, 2016
  4. Simon RabourgMay 30, 2016
  5. Matthieu MoyMay 30, 2016
  6. Michael HaggertyMay 31, 2016
  7. Simon RabourgMay 31, 2016
  8. 2/2 strbuf: allow to use preallocated memoryWilliam Duclot, May 30, 2016
  9. Johannes SchindelinMay 30, 2016
  10. William DuclotMay 30, 2016
  11. Johannes SchindelinMay 31, 2016
  12. Michael HaggertyMay 31, 2016
  13. Johannes SchindelinMay 31, 2016
  14. Michael HaggertyMay 31, 2016
  15. Matthieu MoyMay 30, 2016
  16. William DuclotMay 30, 2016
  17. Matthieu MoyMay 30, 2016
  18. William DuclotMay 30, 2016
  19. Michael HaggertyMay 31, 2016
  20. William DuclotMay 31, 2016
  21. William DuclotJun 3, 2016
  22. Mike HommeyMay 30, 2016
  23. William DuclotMay 30, 2016
  24. Mike HommeyMay 30, 2016
  25. Junio C HamanoMay 31, 2016
  26. WilliamMay 31, 2016
  27. Matthieu MoyMay 31, 2016
  28. William DuclotMay 31, 2016
  29. Remi Galan AlfonsoMay 30, 2016
  30. Jeff KingJun 1, 2016
  31. David TurnerJun 1, 2016
  32. Jeff KingJun 1, 2016
  33. David TurnerJun 1, 2016
  34. Jeff KingJun 1, 2016
  35. Michael HaggertyJun 2, 2016
  36. Matthieu MoyJun 2, 2016
  37. William DuclotJun 2, 2016
  38. Jeff KingJun 24, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.