Open ash2k opened 5 days ago
I think you're totally right on this one, there's definitely an edge case here. That being said, it's only an edge case in the specific code path you mentioned, i.e. some buffer is pulled from the pool, but only partially filled. If I did this:
buf := make([]byte, 32*1034)
_, _ = reader.Read(buf)
buffer := mem.NewBuffer(buf, pool)
buffer.Free()
Then the buffer would get put into the pool. I guess this is far less likely, in this case, the pool
argument should be nil
, making this really a non-issue.
Stepping back for a bit though, is there a reason you are requesting large buffers but reading very little data in them? Is it because you have no way of knowing ahead of time how much data you will be reading from the reader? Is it not possible to request smaller buffers?
I think either way, fixing the size check here makes sense to me
is there a reason you are requesting large buffers but reading very little data in them? Is it because you have no way of knowing ahead of time how much data you will be reading from the reader?
Yes, I don't know how much data there will be. It depends. Can be nothing, can be up to a few MB. It's a somewhat generic piece of code.
Is it not possible to request smaller buffers?
It is but then the overhead is bigger. I think it should be cheaper to use a ~single buffer size across many such places in the codebase. That makes the chance of reusing the buffer much higher since it's of the same capacity. A lot of places in my code use 32KiB, so this codepath also does that.
What version of gRPC are you using?
1.66.2
What version of Go are you using (
go version
)?N/A
What operating system (Linux, Windows, …) and version?
N/A
What did you do?
Roughly this:
I'm getting a ~big buffer from the pool and reading into it.
What did you expect to see?
I expect to get a "normal" buffer for the
buf
slice. That way it can be put back into the pool when it's no longer needed.What did you see instead?
mem.NewBuffer()
looks at the length of thebuf
and, some times, it's under the threshold. In that case thebuf
is wrapped intoSliceBuffer
and returned as is. This meansbuf
will not be returned into the pool, but become garbage. This defeats the point of buffer pooling, obviously.Perhaps
mem.NewBuffer()
should look at slice capacity (cap(buf)
) instead of length (len(buf)
)?Current (undocumented) behavior contradicts the function documentation too:
The buffer will not be returned to the pool if it's length is too small.