stats: Re-use objects while calling multiple Handlers #8639
Conversation
arjan-bal
changed the title
arjan-bal
added
Type: Performance
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8639 +/- ##
==========================================
+ Coverage 81.28% 81.95% +0.67%
==========================================
Files 415 416 +1
Lines 40888 40760 -128
==========================================
+ Hits 33234 33404 +170
+ Misses 6162 5995 -167
+ Partials 1492 1361 -131
🚀 New features to boost your workflow:
|
arjan-bal
force-pushed
the
stats-handlers-reuse-event-objects
branch
2 times, most recently
from
a36c3d1 to
88fb8c2
Compare
arjan-bal
force-pushed
the
stats-handlers-reuse-event-objects
branch
from
88fb8c2 to
38cc04a
Compare
|
Do you have any benchmark numbers for this change? |
There was a problem hiding this comment.
Also, I was wondering what if you exported the combinedHandler and used it as a concrete type instead of as stats.Handler? The grpc server and the channel could use this concrete type, and any other internal type could as well. For external facing APIs that are expecting a stats.Handler, it should still continue to work if the caller has the concrete type. But again, I don't know if this will have any bearing on the performance. So, if this is going to increase the scope of the PR, please push back. Thanks.
| } | ||
| for _, sh := range t.statsHandlers { | ||
| t.ctx = sh.TagConn(t.ctx, &stats.ConnTagInfo{ | ||
| if t.statsHandler != nil { |
There was a problem hiding this comment.
What if we made the combined handler's methods work for a nil receiver? It will remove a bunch of nil checks, but I don't know if or how that affects performance.
There was a problem hiding this comment.
Hmm .. I guess the nil check is much cheaper that a function call. So, I guess we are good. But if you have more insights, please share.
| for _, h := range ch.handlers { | ||
| ctx = h.TagRPC(ctx, info) | ||
| } |
There was a problem hiding this comment.
All methods of this combinedHandler still iterates through the list of handlers and invokes them in turn. So, how does this not result in heap allocations while the previous approach did?
| header, ok := metadata.FromOutgoingContext(ctx) | ||
| if ok { | ||
| header.Set("user-agent", t.userAgent) | ||
| } else { | ||
| header = metadata.Pairs("user-agent", t.userAgent) | ||
| } |
There was a problem hiding this comment.
It's interesting that we add this header only when stats handlers are configured. Do you happen to know why?
| "google.golang.org/grpc/keepalive" | ||
| "google.golang.org/grpc/resolver" | ||
| "google.golang.org/grpc/serviceconfig" | ||
| extstats "google.golang.org/grpc/stats" |
There was a problem hiding this comment.
This rename, extstats, is very similar to expstats which we use in a bunch of other places to import experimental/stats package. Can we come up with a better name?
3 participants
This PR improves performance by eliminating heap allocations when multiple stats handlers are configured.
Previously, iterating through a list of handlers caused one heap allocation per handler for each RPC. This change introduces a Handler that combines multiple Handlers and implements the
Handlerinterface. The combined handler delegates calls to the handlers it contains.This approach allows gRPC clients and servers to operate as if there were only a single
Handlerregistered, simplifying the internal logic and removing the per-RPC allocation overhead. To avoid any performance impact when stats are disabled, the combinedHandleris only created when at least one handler is registered.RELEASE NOTES: N/A