-
Notifications
You must be signed in to change notification settings - Fork 4.6k
transport: Replace closures with interfaces to avoid heap allocations #8630
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8630 +/- ##
==========================================
- Coverage 82.17% 82.03% -0.15%
==========================================
Files 415 415
Lines 40709 40726 +17
==========================================
- Hits 33453 33409 -44
- Misses 5883 5933 +50
- Partials 1373 1384 +11
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general this approach LGTM. One question to maybe simplify things. Thanks for the improvements.
// Callback to state application's intentions to read data. This | ||
// is used to adjust flow control, if needed. | ||
requestRead func(int) | ||
readRequester readRequester |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to embed some of these things instead so that we don't have to implement the trampoline methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updateWindow
method on the new windowHandler
interface takes a single parameter, while the corresponding methods on http2Client
and http2Server
expect a second parameter for the stream itself. The intermediate methods on ServerStream
and ClientStream
supply this extra parameter before calling the underlying http2
methods.
func (s *ServerStream) updateWindow(n int) {
// The receiver 's' is passed as the required stream parameter.
s.st.updateWindow(s, uint32(n))
}
I couldn't find a simple way to remove these one-line wrapper methods without changing the windowHandler
interface to accept the extra *Stream
parameter. That change would require callers like transportReader
to hold a reference to the stream just for this call, which feels like a worse design.
In Go, creating a closure results in a heap allocation if the compiler determines the closure might outlive the function in which it was created. This change removes two such closures, replacing them with interfaces that are implemented by the
ClientStream
andServerStream
structs.While this pattern may slightly reduce readability, the performance benefit is worthwhile, as this transport code is executed for every new stream. This reduces allocs/unary RPC by 2.5%.
Testing
RELEASE NOTES: N/A