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

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

From
Junio C Hamano <gitster@pobox.com>
Date
Jun 6, 2016, 22:44 UTC
Message-ID
<xmqqfusquedk.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<20160606203901.GA7667@Messiaen>
William Duclot <william.duclot@ensimag.grenoble-inp.fr> writes:
> I'm not sure to follow you. I agree that the "fixed strbuf" feature is
> flawed by the presence of this `die()`. But (unless misunderstanding)
> the "owns_memory" bit you talk about does exist in this patch, and allow
> the exact behavior you describe.

Imagine that I know most of my input lines are shorter than 80 bytes and definitely shorter than 128 bytes. I may want to say:

	/* allocate initial buffer ch[128] and attach it to line */
	struct strbuf line = STRBUF_INIT_ON_STACK(128);
	while (!strbuf_getline(&line, stdin)) {
		... use contents of &line ...
	}
        strbuf_release(&line);

knowing that I won't waste too much cycles and memory from heap most of the time. Further imagine that one line in the input happened to be 200 bytes long. After processing that line, the next call to strbuf_getline() will call strbuf_reset(&line).

I think that call should reset line.buf to the original buffer on the stack, instead of saying "Ok, I'll ignore the original memory not owned by us and instead keep pointing at the allocated memory", as the allocation was done as a fallback measure. The reason why strbuf_reset() is different from strbuf_release() is because we anticipate that a buffer that is reset is going to be used again very soon and want to avoid freeing only to call another alloc immediately. We just set its logical length to 0 without shrinking what has already been allocated in the heap, because there is no good hint that tells us what the optimal size of an empty buffer in the particular caller that is waiting to be used before this "improve API" effort.

But that "lack of hint" no longer is true with this "imporve API" thing. We _know_ that the caller thinks the anticipated typical workload should fit well within the 128 bytes buffer that it originally gave us. That is what I'd expect from an "initialize a reasonably sized piece of memory on stack and use it most of the time in strbuf" feature.

> The patch allow to use a preallocated strbuf OR a preallocated-and-fixed
> strbuf. Does the "fixed" part is useful, that's very debatable.

"Fixed and non-extendable" is useless; there is no room for debating.

Previous: William DuclotNext: Jeff King
Message 12 of 32 in “strbuf: improve API”
  1. 0/3 strbuf: improve APIWilliam Duclot, Jun 6, 2016
  2. 1/3 strbuf: add testsWilliam Duclot, Jun 6, 2016
  3. Matthieu MoyJun 6, 2016
  4. Johannes SchindelinJun 7, 2016
  5. 2/3 pretty.c: rename strbuf_wrap() functionWilliam Duclot, Jun 6, 2016
  6. Matthieu MoyJun 6, 2016
  7. Johannes SchindelinJun 7, 2016
  8. 3/3 strbuf: allow to use preallocated memoryWilliam Duclot, Jun 6, 2016
  9. Matthieu MoyJun 6, 2016
  10. Junio C HamanoJun 6, 2016
  11. William DuclotJun 6, 2016
  12. Junio C HamanoJun 6, 2016
  13. Jeff KingJun 6, 2016
  14. Junio C HamanoJun 6, 2016
  15. Junio C HamanoJun 6, 2016
  16. Jeff KingJun 6, 2016
  17. William DuclotJun 7, 2016
  18. Junio C HamanoJun 7, 2016
  19. Michael HaggertyJun 8, 2016
  20. Junio C HamanoJun 8, 2016
  21. Jeff KingJun 8, 2016
  22. send-pack: use buffered I/O to talk to pack-objectsJeff King, Jun 8, 2016
  23. Matthieu MoyJun 9, 2016
  24. Ramsay JonesJun 9, 2016
  25. Jeff KingJun 9, 2016
  26. Ramsay JonesJun 9, 2016
  27. Junio C HamanoJun 9, 2016
  28. Jeff KingJun 9, 2016
  29. Matthieu MoyJun 9, 2016
  30. Junio C HamanoJun 8, 2016
  31. Jeff KingJun 8, 2016
  32. Junio C HamanoJun 8, 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.