transport: Replace closures with interfaces to avoid heap allocations#8630
transport: Replace closures with interfaces to avoid heap allocations#8630arjan-bal merged 4 commits intogrpc:masterfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8630 +/- ##
==========================================
- Coverage 81.28% 79.42% -1.86%
==========================================
Files 415 415
Lines 40888 41339 +451
==========================================
- Hits 33234 32835 -399
- Misses 6162 6626 +464
- Partials 1492 1878 +386
🚀 New features to boost your workflow:
|
dfawley
left a comment
There was a problem hiding this comment.
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.
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.
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
ClientStreamandServerStreamstructs.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