mem.NewBuffer() should look at slice capacity?

[ash2k]

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:

buf := pool.Get(32*1024)

_, _ = reader.Read(buf)

buffer := mem.NewBuffer(buf, pool)

buffer.Free()

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 the buf and, some times, it's under the threshold. In that case the buf is wrapped into SliceBuffer and returned as is. This means buf 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:

// NewBuffer creates a new Buffer from the given data, initializing the reference
// counter to 1. The data will then be returned to the given pool when all
// references to the returned Buffer are released. As a special case to avoid
// additional allocations, if the given buffer pool is nil, the returned buffer
// will be a "no-op" Buffer where invoking Buffer.Free() does nothing and the
// underlying data is never freed.
//
// Note that the backing array of the given data is not copied.

The buffer will not be returned to the pool if it's length is too small.

[PapaCharlie]

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

[ash2k]

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.