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

William Duclot [off-list ref] 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.

On Mon, Jun 06, 2016 at 03:44:07PM -0700, Junio C Hamano wrote:

William Duclot [off-list ref] writes:

quoted

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.

I am not sure I agree. Do we think accessing the stack buffer is somehow
cheaper than the heap buffer (perhaps because of cache effects)? If so,
how much cheaper?

I think you can model reusing an already-allocated heap buffer as a
hit/miss type of scenario. A "hit" means we see a larger-than-128 line
and can avoid the allocation cost by reusing the heap buffer. A "miss"
means the line is less than 128, and we pay the cost to use the heap
instead of the stack, whatever that is.

My suspicion is that the cost of a miss is essentially zero, so the best
strategy is to optimize for as many hits as possible (once the cost of
the initial allocation has been paid, though I am still not even
convinced that is a meaningful amount, especially in a loop like this
where we can so easily reuse a heap buffer).

-Peff