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.
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
envelopeReaderis 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.