Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Release buffer for end-stream messages back to the pool #678

Merged
merged 2 commits into from
Feb 13, 2024

Conversation

jhump
Copy link
Member

@jhump jhump commented Feb 3, 2024

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.

envelope.go Show resolved Hide resolved
envelope.go Show resolved Hide resolved
protocol_grpc.go Show resolved Hide resolved
@jhump jhump merged commit 1061b35 into main Feb 13, 2024
11 checks passed
@jhump jhump deleted the jh/free-up-memory branch February 13, 2024 19:49
@jhump jhump added the enhancement New feature or request label Feb 16, 2024
@jhump jhump mentioned this pull request Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants