[ISSUE #10441] Reduce per-RPC allocation in metrics by caching static AttributeKey instances#10443
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR focuses on reducing allocation/CPU overhead on metrics and remoting hot paths by introducing AttributeKey singletons and adding small caches for frequently repeated attribute/metric lookups.
Changes:
- Added fast-path caching for remoting request/response code distribution counting.
- Introduced typed OpenTelemetry
AttributeKeyconstants and attribute caching/build helpers in remoting and broker metrics. - Reduced repeated string allocations (e.g., cached lowercase metrics value, avoided
Map.getOrDefaultboxing paths).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| remoting/src/main/java/org/apache/rocketmq/remoting/netty/RemotingCodeDistributionHandler.java | Adds “last code” fast path to reduce map lookups for hot request/response codes. |
| remoting/src/main/java/org/apache/rocketmq/remoting/metrics/RemotingMetricsManager.java | Adds attribute caching and typed label keys to reduce OpenTelemetry attribute overhead. |
| remoting/src/main/java/org/apache/rocketmq/remoting/metrics/RemotingMetricsConstant.java | Introduces typed AttributeKey singletons for labels. |
| remoting/src/main/java/org/apache/rocketmq/remoting/common/RemotingHelper.java | Minor optimization to avoid getOrDefault overhead for code descriptions. |
| common/src/main/java/org/apache/rocketmq/common/attribute/TopicMessageType.java | Caches lowercase metrics value to avoid repeated conversions. |
| broker/src/main/resources/rmq.broker.logback.xml | Suppresses Logback status output via NopStatusListener. |
| broker/src/main/java/org/apache/rocketmq/broker/metrics/PopMetricsManager.java | Switches to typed label keys for attributes. |
| broker/src/main/java/org/apache/rocketmq/broker/metrics/BrokerMetricsManager.java | Adds topic attributes cache and switches to typed label keys. |
| broker/src/main/java/org/apache/rocketmq/broker/metrics/BrokerMetricsConstant.java | Introduces typed AttributeKey singletons for broker metrics labels. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
607c58a to
52c49e8
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #10443 +/- ##
=============================================
- Coverage 48.05% 47.95% -0.10%
+ Complexity 13311 13288 -23
=============================================
Files 1377 1377
Lines 100632 100707 +75
Branches 12995 13006 +11
=============================================
- Hits 48359 48295 -64
- Misses 46351 46467 +116
- Partials 5922 5945 +23 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
oss-sentinel-ai
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
This PR caches OpenTelemetry AttributeKey instances as static finals to avoid per-RPC allocation, optimizes TopicMessageType lookup, adds a volatile inline cache for RemotingCodeDistributionHandler, and adds Logback NopStatusListener to suppress startup log noise.
Findings
- [Good] BrokerMetricsConstant.java:69-86 — Pre-built typed
AttributeKeysingletons eliminate per-call allocation on the metrics hot path. - [Good] BrokerMetricsManager.java:105-120 — Import changes correctly reference the new cached keys.
- [Good] RemotingCodeDistributionHandler.java:35-68 — Volatile inline cache pattern correctly avoids
ConcurrentHashMap.computeIfAbsent()on repeated request codes. - [Good] TopicMessageType.java — Enum-based lookup is more efficient than string comparison.
- [Good] rmq.broker.logback.xml —
NopStatusListenersuppresses Logback startup noise in production.
Suggestions
- The volatile inline cache pattern in
RemotingCodeDistributionHandleris effective for sequential workloads. Consider documenting that this optimization assumes temporal locality in request codes. - Consider adding a microbenchmark to quantify the allocation reduction.
Verdict
✅ Approved — Solid performance optimization with clean implementation.
Automated review by github-manager-bot
52c49e8 to
d61959d
Compare
…static AttributeKey instances Cache OpenTelemetry AttributeKey instances as static finals instead of creating new instances per RPC call. Also optimize TopicMessageType lookup, add volatile inline cache for RemotingCodeDistributionHandler, and add Logback NopStatusListener to suppress startup log noise.
d61959d to
994bf3d
Compare
Cache OpenTelemetry AttributeKey instances as static finals instead of creating new instances per RPC call. Also optimize TopicMessageType lookup, add volatile inline cache for RemotingCodeDistributionHandler, and add Logback NopStatusListener to suppress startup log noise.
Which Issue(s) This PR Fixes
AttributeKeyinstances #10441Brief Description
Cache OpenTelemetry
AttributeKeyinstances as static finals instead of creating new ones per RPC call. JFR heap dump shows 38,182AttributeKeyinstances with only 6 distinct key names — each RPC allocates 3-4 throwaway key objects.Changes:
BrokerMetricsConstant/RemotingMetricsConstant:Stringlabels →AttributeKey<T>static finalsBrokerMetricsManager/RemotingMetricsManager/PopMetricsManager: useput(AttributeKey, value)overloadsRemotingCodeDistributionHandler: volatile inline cache for counter methodsTopicMessageType: direct if-else lookup replacingvalues()iterationrmq.broker.logback.xml: addNopStatusListenerHow Did You Test This Change?