[ISSUE #10490] Eliminate per-RPC allocation from Logback status, TopicMessageType toLowerCase, and RemotingHelper eager evaluation#10491
Conversation
…static AttributeKey instances
…, TopicMessageType toLowerCase, and RemotingHelper eager evaluation
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 reduces per-call allocations in hot paths by introducing reusable OpenTelemetry AttributeKey singletons for metrics labels and by avoiding eager default-string creation in request/response code formatting.
Changes:
- Add typed
AttributeKeylabel constants for broker/remoting metrics and migrate selected call sites to use them. - Optimize request/response code description helpers to avoid unnecessary
String.valueOf(...)allocations when a mapping exists. - Precompute/canonicalize metric label values and adjust logging configuration behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| remoting/src/main/java/org/apache/rocketmq/remoting/metrics/RemotingMetricsManager.java | Switches attribute building to use typed AttributeKey constant for protocol type. |
| remoting/src/main/java/org/apache/rocketmq/remoting/metrics/RemotingMetricsConstant.java | Introduces reusable typed AttributeKey singletons for remoting metric labels. |
| remoting/src/main/java/org/apache/rocketmq/remoting/common/RemotingHelper.java | Avoids eager default-string construction in code-to-description helpers. |
| common/src/main/java/org/apache/rocketmq/common/attribute/TopicMessageType.java | Caches lowercase metrics label value to avoid repeated toLowerCase() and enforce Locale.ROOT. |
| broker/src/main/resources/rmq.broker.logback.xml | Adds a NopStatusListener, changing visibility of Logback status output. |
| broker/src/main/java/org/apache/rocketmq/broker/metrics/PopMetricsManager.java | Migrates selected label puts to typed AttributeKey constants. |
| broker/src/main/java/org/apache/rocketmq/broker/metrics/BrokerMetricsManager.java | Migrates multiple metrics label puts to typed AttributeKey constants. |
| broker/src/main/java/org/apache/rocketmq/broker/metrics/BrokerMetricsConstant.java | Introduces reusable typed AttributeKey singletons for broker metric labels. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| <configuration scan="true" scanPeriod="30 seconds"> | ||
|
|
||
| <statusListener class="ch.qos.logback.core.status.NopStatusListener"/> |
| // Pre-built typed AttributeKey singletons. Use these in AttributesBuilder.put() | ||
| // on hot paths to avoid allocating a fresh InternalAttributeKeyImpl per call. |
| // Pre-built typed AttributeKey singletons. Use these in AttributesBuilder.put() | ||
| // on hot paths to avoid allocating a fresh InternalAttributeKeyImpl per call. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #10491 +/- ##
=============================================
- Coverage 48.06% 48.03% -0.04%
- Complexity 13313 13322 +9
=============================================
Files 1377 1377
Lines 100632 100729 +97
Branches 12995 13012 +17
=============================================
+ Hits 48369 48384 +15
- Misses 46344 46393 +49
- Partials 5919 5952 +33 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
This PR eliminates three per-RPC micro-allocations on the hot path: Logback status listener overhead, TopicMessageType.metricsValue repeated toLowerCase(), and RemotingHelper eager String.valueOf() evaluation. Additionally, it pre-builds OTel AttributeKey singletons to avoid per-call InternalAttributeKeyImpl allocation.
Findings
-
[Info]
TopicMessageType.java— CachingmetricsValuewithLocale.ROOTis correct and avoids per-call allocation. Good use ofLocale.ROOTfor deterministic casing. -
[Info]
RemotingHelper.java— Theget()+ null check pattern replacinggetOrDefault(code, String.valueOf(code))is a valid optimization to avoid eager evaluation. Consider adding a brief inline comment explaining why the null-check pattern is used instead ofgetOrDefault, since the intent is not immediately obvious to readers. -
[Info]
BrokerMetricsConstant.java/RemotingMetricsConstant.java— Pre-buildingAttributeKeysingletons is a sound optimization for OTel hot paths. The alignment style (extra spaces for=) is consistent within the block but may trigger checkstyle rules if the project enforces no-multi-spaces. Worth verifying CI passes. -
[Warning] The PR description mentions this is stacked on PR #10443. The diff currently includes 8 files (5 extra from #10443). Reviewers should focus on the 3 core delta files. Please rebase onto
developafter #10443 merges to keep the diff clean.
Suggestions
- Consider adding a microbenchmark (JMH) to quantify the per-RPC allocation reduction, especially for the
AttributeKeysingleton optimization. This would help justify the change and prevent regression. - The
NopStatusListeneraddition is fine, but consider documenting in a comment why it was added (suppress Logback internal status events to avoid per-RPC allocation).
Verdict
Clean performance optimization with no correctness concerns. The changes are well-targeted and low-risk.
Automated review by github-manager-bot
Summary
Three independent per-RPC micro-allocations eliminated:
1. Logback
NopStatusListenerAdd
<statusListener class="ch.qos.logback.core.status.NopStatusListener"/>tormq.broker.logback.xmlto suppress Logback internal status event callbacks.2.
TopicMessageType.metricsValuecachegetMetricsValue()was callingvalue.toLowerCase()per invocation. Now cachesmetricsValueas afinalfield withLocale.ROOT.3.
RemotingHelpereager evaluation fixMap.getOrDefault(code, String.valueOf(code))eagerly evaluatesString.valueOf(code)even on cache hit. Changed toget()+ null check.Dependency
Stacked on PR #10443. Currently includes #10443 changes in the diff; will rebase on
developafter #10443 merges to show only the 3 files above.Files (delta on top of #10443)
broker/src/main/resources/rmq.broker.logback.xmlcommon/.../attribute/TopicMessageType.javametricsValuecached field withLocale.ROOTremoting/.../common/RemotingHelper.javagetOrDefault→get+ null check