Release buffer for end-stream messages back to the pool #678
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I just happened to notice while I was trawling through the grpc-web implementation for other reasons. I thought it might help to skip the copy and actually release this buffer back to the pool.
Looking at benchmark output, it at least does no harm. And the reduction in allocations does show up as it consistently performs ~0.5% fewer for Connect streams and gRPC-web calls (both unary and stream). Strangely, this reduction in allocations doesn't show up for bidi streams in either Connect or gRPC-Web 🤔. The benchmark results in terms of latency and bytes allocated is basically a wash -- no discernible difference (though admittedly, these results have greater volatility from run-to-run than the number of allocations 🤷).
Anyhow, since it doesn't make any significant different, I'm fine if you'd rather close this. But it doesn't seem bad to me -- it doesn't really complicate the code. This also fixes a potential source of pinning memory to the heap: the
envelopeReader
is kept around for the life of the stream, and we weren't clearing the reference to the final*bytes.Buffer
, which would pin it to the heap. Admittedly that likely doesn't make a big difference in practice, since this is the final message in the stream, so it's unlikely that the stream would survive much longer. But it still seemed like reasonable pointer hygiene.