You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "wang-jiahua (via GitHub)" <gi...@apache.org> on 2026/06/08 09:21:09 UTC
[PR] [ISSUE #10441] Reduce per-RPC allocation in metrics by caching static `AttributeKey` instances [rocketmq]
wang-jiahua opened a new pull request, #10443:
URL: https://github.com/apache/rocketmq/pull/10443
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.
<!-- Please make sure the target branch is right. In most case, the target branch should be `develop`. -->
### Which Issue(s) This PR Fixes
<!-- Please ensure that the related issue has already been created, and [link this pull request to that issue using keywords](<https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword>) to ensure automatic closure. -->
- Fixes #10441
### Brief Description
<!-- Write a brief description for your pull request to help the maintainer understand the reasons behind your changes. -->
Cache OpenTelemetry `AttributeKey` instances as static finals instead of creating new ones per RPC call. JFR heap dump shows 38,182 `AttributeKey` instances with only 6 distinct key names — each RPC allocates 3-4 throwaway key objects.
Changes:
- `BrokerMetricsConstant` / `RemotingMetricsConstant`: `String` labels → `AttributeKey<T>` static finals
- `BrokerMetricsManager` / `RemotingMetricsManager` / `PopMetricsManager`: use `put(AttributeKey, value)` overloads
- `RemotingCodeDistributionHandler`: volatile inline cache for counter methods
- `TopicMessageType`: direct if-else lookup replacing `values()` iteration
- `rmq.broker.logback.xml`: add `NopStatusListener`
### How Did You Test This Change?
<!-- In order to ensure the code quality of Apache RocketMQ, we expect every pull request to have undergone thorough testing. -->
- JFR profiling (ObjectAllocationSample) on 4×ECS (8c/30G), 128 producer threads, 1KB body, Temurin JDK 21
- Verified all existing metrics report identically before/after
- Zero functional change — only metric label construction paths affected
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #10441] Reduce per-RPC allocation in metrics by caching static `AttributeKey` instances [rocketmq]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10443:
URL: https://github.com/apache/rocketmq/pull/10443#issuecomment-4649205679
## [Codecov](https://app.codecov.io/gh/apache/rocketmq/pull/10443?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `28.82883%` with `79 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 47.95%. Comparing base ([`82a6a78`](https://app.codecov.io/gh/apache/rocketmq/commit/82a6a784ee35b36d94b3ab644bd5394834e2c540?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`52c49e8`](https://app.codecov.io/gh/apache/rocketmq/commit/52c49e8c2b689b2bf85da1cda1ab0145572b2500?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/rocketmq/pull/10443?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../rocketmq/broker/metrics/BrokerMetricsManager.java](https://app.codecov.io/gh/apache/rocketmq/pull/10443?src=pr&el=tree&filepath=broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Frocketmq%2Fbroker%2Fmetrics%2FBrokerMetricsManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9icm9rZXIvbWV0cmljcy9Ccm9rZXJNZXRyaWNzTWFuYWdlci5qYXZh) | 4.76% | [40 Missing :warning: ](https://app.codecov.io/gh/apache/rocketmq/pull/10443?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ketmq/remoting/metrics/RemotingMetricsManager.java](https://app.codecov.io/gh/apache/rocketmq/pull/10443?src=pr&el=tree&filepath=remoting%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Frocketmq%2Fremoting%2Fmetrics%2FRemotingMetricsManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cmVtb3Rpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3JlbW90aW5nL21ldHJpY3MvUmVtb3RpbmdNZXRyaWNzTWFuYWdlci5qYXZh) | 0.00% | [24 Missing :warning: ](https://app.codecov.io/gh/apache/rocketmq/pull/10443?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...etmq/remoting/metrics/RemotingMetricsConstant.java](https://app.codecov.io/gh/apache/rocketmq/pull/10443?src=pr&el=tree&filepath=remoting%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Frocketmq%2Fremoting%2Fmetrics%2FRemotingMetricsConstant.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cmVtb3Rpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3JlbW90aW5nL21ldHJpY3MvUmVtb3RpbmdNZXRyaWNzQ29uc3RhbnQuamF2YQ==) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/rocketmq/pull/10443?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...che/rocketmq/broker/metrics/PopMetricsManager.java](https://app.codecov.io/gh/apache/rocketmq/pull/10443?src=pr&el=tree&filepath=broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Frocketmq%2Fbroker%2Fmetrics%2FPopMetricsManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9icm9rZXIvbWV0cmljcy9Qb3BNZXRyaWNzTWFuYWdlci5qYXZh) | 33.33% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/rocketmq/pull/10443?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...pache/rocketmq/remoting/common/RemotingHelper.java](https://app.codecov.io/gh/apache/rocketmq/pull/10443?src=pr&el=tree&filepath=remoting%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Frocketmq%2Fremoting%2Fcommon%2FRemotingHelper.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cmVtb3Rpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3JlbW90aW5nL2NvbW1vbi9SZW1vdGluZ0hlbHBlci5qYXZh) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/rocketmq/pull/10443?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...emoting/netty/RemotingCodeDistributionHandler.java](https://app.codecov.io/gh/apache/rocketmq/pull/10443?src=pr&el=tree&filepath=remoting%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Frocketmq%2Fremoting%2Fnetty%2FRemotingCodeDistributionHandler.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cmVtb3Rpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3JlbW90aW5nL25ldHR5L1JlbW90aW5nQ29kZURpc3RyaWJ1dGlvbkhhbmRsZXIuamF2YQ==) | 85.71% | [0 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/rocketmq/pull/10443?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ 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
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/rocketmq/pull/10443?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #10441] Reduce per-RPC allocation in metrics by caching static `AttributeKey` instances [rocketmq]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #10443:
URL: https://github.com/apache/rocketmq/pull/10443#discussion_r3372047512
##########
remoting/src/main/java/org/apache/rocketmq/remoting/netty/RemotingCodeDistributionHandler.java:
##########
@@ -32,19 +32,35 @@ public class RemotingCodeDistributionHandler extends ChannelDuplexHandler {
private final ConcurrentMap<Integer, LongAdder> inboundDistribution;
private final ConcurrentMap<Integer, LongAdder> outboundDistribution;
+ private volatile int lastInCode = Integer.MIN_VALUE;
+ private volatile LongAdder lastInAdder;
+ private volatile int lastOutCode = Integer.MIN_VALUE;
+ private volatile LongAdder lastOutAdder;
public RemotingCodeDistributionHandler() {
inboundDistribution = new ConcurrentHashMap<>();
outboundDistribution = new ConcurrentHashMap<>();
}
private void countInbound(int requestCode) {
+ if (requestCode == lastInCode) {
+ lastInAdder.increment();
+ return;
+ }
LongAdder item = inboundDistribution.computeIfAbsent(requestCode, k -> new LongAdder());
+ lastInCode = requestCode;
+ lastInAdder = item;
item.increment();
}
private void countOutbound(int responseCode) {
+ if (responseCode == lastOutCode) {
+ lastOutAdder.increment();
+ return;
+ }
LongAdder item = outboundDistribution.computeIfAbsent(responseCode, k -> new LongAdder());
+ lastOutCode = responseCode;
+ lastOutAdder = item;
item.increment();
}
Review Comment:
The fast-path cache updates `last*Code` before `last*Adder`. If this handler instance is ever accessed concurrently (e.g., shared handler, or cross-thread calls), another thread can observe the new code but still see a stale/null adder, leading to incorrect counting or NPE. Update ordering should ensure the adder write happens-before the code is published (e.g., assign `last*Adder` first, then `last*Code`, and consider guarding the fast path with `last*Adder != null`). Also consider the sentinel initialization: if a code can equal `Integer.MIN_VALUE`, the initial fast-path hit would NPE.
##########
remoting/src/main/java/org/apache/rocketmq/remoting/metrics/RemotingMetricsManager.java:
##########
@@ -86,6 +97,44 @@ public List<Pair<InstrumentSelector, ViewBuilder>> getMetricsView() {
return Lists.newArrayList(new Pair<>(selector, viewBuilder));
}
+ public Attributes getOrBuildAttributes(int requestCode, int responseCode,
+ boolean isLongPolling, String result) {
+ int resultIdx;
+ if (result == RESULT_SUCCESS) resultIdx = 0;
+ else if (result == RESULT_ONEWAY) resultIdx = 1;
+ else if (result == RESULT_WRITE_CHANNEL_FAILED) resultIdx = 2;
+ else if (result == RESULT_CANCELED) resultIdx = 3;
+ else resultIdx = -1;
Review Comment:
String comparison uses reference equality (`==`), which can fail when `result` is a different String instance with the same content. This will incorrectly bypass caching (and change the computed `resultIdx`). Use content comparison (e.g., `RESULT_SUCCESS.equals(result)`) or a switch on the string value.
##########
common/src/main/java/org/apache/rocketmq/common/attribute/TopicMessageType.java:
##########
@@ -67,6 +69,6 @@ public static TopicMessageType parseFromMessageProperty(Map<String, String> mess
}
public String getMetricsValue() {
- return value.toLowerCase();
+ return metricsValue;
}
Review Comment:
Using `String.toLowerCase()` without an explicit locale makes the metrics label value depend on the JVM default locale (and this change also locks that value at enum initialization time). To make metrics stable across environments, use `toLowerCase(Locale.ROOT)` when computing `metricsValue`.
##########
broker/src/main/java/org/apache/rocketmq/broker/metrics/BrokerMetricsManager.java:
##########
@@ -195,12 +202,31 @@ public AttributesBuilder newAttributesBuilder() {
return attributesBuilder;
}
+ public Attributes getOrBuildTopicAttributes(String topic, String messageType, boolean isSystem) {
+ Attributes lastAttrs = this.lastTopicAttributes;
+ if (lastAttrs != null && topic.equals(this.lastTopicName) && messageType.equals(this.lastTopicMsgType)) {
+ return lastAttrs;
+ }
+ String cacheKey = topic + '|' + messageType;
+ Attributes attrs = topicAttributesCache.computeIfAbsent(cacheKey, k ->
+ newAttributesBuilder()
+ .put(LABEL_TOPIC_KEY, topic)
+ .put(LABEL_MESSAGE_TYPE_KEY, messageType)
+ .put(LABEL_IS_SYSTEM_KEY, isSystem)
+ .build()
+ );
+ this.lastTopicName = topic;
+ this.lastTopicMsgType = messageType;
+ this.lastTopicAttributes = attrs;
+ return attrs;
+ }
Review Comment:
The cache key and the last-value fast path ignore `isSystem`, even though `LABEL_IS_SYSTEM_KEY` is included in the built Attributes. If `isSystem` differs for the same `(topic, messageType)` across calls, this will return Attributes with the wrong `isSystem` label value. Include `isSystem` in both the fast-path comparison and the cache key (or maintain separate caches per `isSystem`).
##########
broker/src/main/resources/rmq.broker.logback.xml:
##########
@@ -18,6 +18,8 @@
<configuration scan="true" scanPeriod="30 seconds">
+ <statusListener class="ch.qos.logback.core.status.NopStatusListener"/>
+
<appender name="DefaultSiftingAppender_inner" class="ch.qos.logback.classic.sift.SiftingAppender">
Review Comment:
Adding `NopStatusListener` suppresses Logback status output, which can hide configuration errors/warnings that are useful during troubleshooting (especially when reloading with `scan=true`). Consider limiting this to specific deployments/profiles, or using a status listener that can be enabled/disabled via configuration rather than hard-disabling status reporting.
##########
common/src/main/java/org/apache/rocketmq/common/attribute/TopicMessageType.java:
##########
@@ -33,9 +33,11 @@ public enum TopicMessageType {
MIXED("MIXED");
private final String value;
+ private final String metricsValue;
TopicMessageType(String value) {
this.value = value;
+ this.metricsValue = value.toLowerCase();
}
Review Comment:
Using `String.toLowerCase()` without an explicit locale makes the metrics label value depend on the JVM default locale (and this change also locks that value at enum initialization time). To make metrics stable across environments, use `toLowerCase(Locale.ROOT)` when computing `metricsValue`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #10441] Reduce per-RPC allocation in metrics by caching static `AttributeKey` instances [rocketmq]
Posted by "wang-jiahua (via GitHub)" <gi...@apache.org>.
wang-jiahua commented on code in PR #10443:
URL: https://github.com/apache/rocketmq/pull/10443#discussion_r3389165135
##########
remoting/src/main/java/org/apache/rocketmq/remoting/netty/RemotingCodeDistributionHandler.java:
##########
@@ -32,19 +32,35 @@ public class RemotingCodeDistributionHandler extends ChannelDuplexHandler {
private final ConcurrentMap<Integer, LongAdder> inboundDistribution;
private final ConcurrentMap<Integer, LongAdder> outboundDistribution;
+ private volatile int lastInCode = Integer.MIN_VALUE;
+ private volatile LongAdder lastInAdder;
+ private volatile int lastOutCode = Integer.MIN_VALUE;
+ private volatile LongAdder lastOutAdder;
public RemotingCodeDistributionHandler() {
inboundDistribution = new ConcurrentHashMap<>();
outboundDistribution = new ConcurrentHashMap<>();
}
private void countInbound(int requestCode) {
+ if (requestCode == lastInCode) {
+ lastInAdder.increment();
+ return;
+ }
LongAdder item = inboundDistribution.computeIfAbsent(requestCode, k -> new LongAdder());
+ lastInCode = requestCode;
+ lastInAdder = item;
item.increment();
}
private void countOutbound(int responseCode) {
+ if (responseCode == lastOutCode) {
+ lastOutAdder.increment();
+ return;
+ }
LongAdder item = outboundDistribution.computeIfAbsent(responseCode, k -> new LongAdder());
+ lastOutCode = responseCode;
+ lastOutAdder = item;
item.increment();
}
Review Comment:
Fixed: introduced an immutable `CachedCounter { final int code; final LongAdder adder; }` holder published via a single volatile write. Racing threads always observe a consistent (code, adder) pair. The `Integer.MIN_VALUE` sentinel issue is also resolved since the cached pair is initialized as `null`, not with a sentinel code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #10441] Reduce per-RPC allocation in metrics by caching static `AttributeKey` instances [rocketmq]
Posted by "wang-jiahua (via GitHub)" <gi...@apache.org>.
wang-jiahua commented on code in PR #10443:
URL: https://github.com/apache/rocketmq/pull/10443#discussion_r3389168535
##########
broker/src/main/java/org/apache/rocketmq/broker/metrics/BrokerMetricsManager.java:
##########
@@ -195,12 +202,31 @@ public AttributesBuilder newAttributesBuilder() {
return attributesBuilder;
}
+ public Attributes getOrBuildTopicAttributes(String topic, String messageType, boolean isSystem) {
+ Attributes lastAttrs = this.lastTopicAttributes;
+ if (lastAttrs != null && topic.equals(this.lastTopicName) && messageType.equals(this.lastTopicMsgType)) {
+ return lastAttrs;
+ }
+ String cacheKey = topic + '|' + messageType;
+ Attributes attrs = topicAttributesCache.computeIfAbsent(cacheKey, k ->
+ newAttributesBuilder()
+ .put(LABEL_TOPIC_KEY, topic)
+ .put(LABEL_MESSAGE_TYPE_KEY, messageType)
+ .put(LABEL_IS_SYSTEM_KEY, isSystem)
+ .build()
+ );
+ this.lastTopicName = topic;
+ this.lastTopicMsgType = messageType;
+ this.lastTopicAttributes = attrs;
+ return attrs;
+ }
Review Comment:
Fixed: `isSystem` is now included in both the inline cache comparison (`isSystem == this.lastTopicIsSystem`) and the `ConcurrentHashMap` cache key (`topic + "|" + messageType + "|" + isSystem`). Calls with different `isSystem` values for the same `(topic, messageType)` pair will correctly return different Attributes.
##########
common/src/main/java/org/apache/rocketmq/common/attribute/TopicMessageType.java:
##########
@@ -33,9 +33,11 @@ public enum TopicMessageType {
MIXED("MIXED");
private final String value;
+ private final String metricsValue;
TopicMessageType(String value) {
this.value = value;
+ this.metricsValue = value.toLowerCase();
}
Review Comment:
Fixed: `metricsValue` is now computed at enum construction time with `value.toLowerCase(Locale.ROOT)`, making it locale-independent and stable across environments.
##########
common/src/main/java/org/apache/rocketmq/common/attribute/TopicMessageType.java:
##########
@@ -67,6 +69,6 @@ public static TopicMessageType parseFromMessageProperty(Map<String, String> mess
}
public String getMetricsValue() {
- return value.toLowerCase();
+ return metricsValue;
}
Review Comment:
Same fix as above — `metricsValue` field uses `toLowerCase(Locale.ROOT)` at construction time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #10441] Reduce per-RPC allocation in metrics by caching static `AttributeKey` instances [rocketmq]
Posted by "wang-jiahua (via GitHub)" <gi...@apache.org>.
wang-jiahua commented on code in PR #10443:
URL: https://github.com/apache/rocketmq/pull/10443#discussion_r3389171081
##########
broker/src/main/resources/rmq.broker.logback.xml:
##########
@@ -18,6 +18,8 @@
<configuration scan="true" scanPeriod="30 seconds">
+ <statusListener class="ch.qos.logback.core.status.NopStatusListener"/>
+
<appender name="DefaultSiftingAppender_inner" class="ch.qos.logback.classic.sift.SiftingAppender">
Review Comment:
This is intentional. NopStatusListener suppresses Logback internal status messages (config parse results) that are written to stderr on every broker startup. In production, the logback config is stable and validated during deployment — these messages add noise to stderr without actionable value. The `scan=true` auto-reload is already guarded by Logback's own error handling (misconfigured reload falls back to the last working config). If troubleshooting a logback config issue, operators can temporarily remove this listener.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #10441] Reduce per-RPC allocation in metrics by caching static `AttributeKey` instances [rocketmq]
Posted by "RongtongJin (via GitHub)" <gi...@apache.org>.
RongtongJin merged PR #10443:
URL: https://github.com/apache/rocketmq/pull/10443
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #10441] Reduce per-RPC allocation in metrics by caching static `AttributeKey` instances [rocketmq]
Posted by "RongtongJin (via GitHub)" <gi...@apache.org>.
RongtongJin commented on code in PR #10443:
URL: https://github.com/apache/rocketmq/pull/10443#discussion_r3379569031
##########
remoting/src/main/java/org/apache/rocketmq/remoting/netty/RemotingCodeDistributionHandler.java:
##########
@@ -32,19 +32,37 @@ public class RemotingCodeDistributionHandler extends ChannelDuplexHandler {
private final ConcurrentMap<Integer, LongAdder> inboundDistribution;
private final ConcurrentMap<Integer, LongAdder> outboundDistribution;
+ private volatile int lastInCode = Integer.MIN_VALUE;
+ private volatile LongAdder lastInAdder;
+ private volatile int lastOutCode = Integer.MIN_VALUE;
+ private volatile LongAdder lastOutAdder;
public RemotingCodeDistributionHandler() {
inboundDistribution = new ConcurrentHashMap<>();
outboundDistribution = new ConcurrentHashMap<>();
}
private void countInbound(int requestCode) {
+ LongAdder adder = lastInAdder;
+ if (requestCode == lastInCode && adder != null) {
+ adder.increment();
+ return;
+ }
LongAdder item = inboundDistribution.computeIfAbsent(requestCode, k -> new LongAdder());
+ lastInAdder = item;
+ lastInCode = requestCode;
item.increment();
Review Comment:
`lastInCode` and `lastInAdder` are updated as separate volatile fields, but this handler is `@Sharable` and can be invoked concurrently by multiple channel event loops. A racing thread can observe a mixed pair, for example the new `lastInAdder` for code 2 while `lastInCode` is still code 1, and then increment the wrong `LongAdder`. Please publish the cached pair atomically, e.g. via one immutable `LastCounter { code, adder }` stored in a single volatile field, or remove this fast path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #10441] Reduce per-RPC allocation in metrics by caching static `AttributeKey` instances [rocketmq]
Posted by "wang-jiahua (via GitHub)" <gi...@apache.org>.
wang-jiahua commented on code in PR #10443:
URL: https://github.com/apache/rocketmq/pull/10443#discussion_r3389166822
##########
remoting/src/main/java/org/apache/rocketmq/remoting/metrics/RemotingMetricsManager.java:
##########
@@ -86,6 +97,44 @@ public List<Pair<InstrumentSelector, ViewBuilder>> getMetricsView() {
return Lists.newArrayList(new Pair<>(selector, viewBuilder));
}
+ public Attributes getOrBuildAttributes(int requestCode, int responseCode,
+ boolean isLongPolling, String result) {
+ int resultIdx;
+ if (result == RESULT_SUCCESS) resultIdx = 0;
+ else if (result == RESULT_ONEWAY) resultIdx = 1;
+ else if (result == RESULT_WRITE_CHANNEL_FAILED) resultIdx = 2;
+ else if (result == RESULT_CANCELED) resultIdx = 3;
+ else resultIdx = -1;
Review Comment:
Fixed: changed from reference equality (`==`) to `.equals()` for all result string comparisons in `getOrBuildAttributes`. The four known result constants are now matched via `RESULT_SUCCESS.equals(result)` etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #10441] Reduce per-RPC allocation in metrics by caching static `AttributeKey` instances [rocketmq]
Posted by "wang-jiahua (via GitHub)" <gi...@apache.org>.
wang-jiahua commented on code in PR #10443:
URL: https://github.com/apache/rocketmq/pull/10443#discussion_r3379618620
##########
remoting/src/main/java/org/apache/rocketmq/remoting/netty/RemotingCodeDistributionHandler.java:
##########
@@ -32,19 +32,37 @@ public class RemotingCodeDistributionHandler extends ChannelDuplexHandler {
private final ConcurrentMap<Integer, LongAdder> inboundDistribution;
private final ConcurrentMap<Integer, LongAdder> outboundDistribution;
+ private volatile int lastInCode = Integer.MIN_VALUE;
+ private volatile LongAdder lastInAdder;
+ private volatile int lastOutCode = Integer.MIN_VALUE;
+ private volatile LongAdder lastOutAdder;
public RemotingCodeDistributionHandler() {
inboundDistribution = new ConcurrentHashMap<>();
outboundDistribution = new ConcurrentHashMap<>();
}
private void countInbound(int requestCode) {
+ LongAdder adder = lastInAdder;
+ if (requestCode == lastInCode && adder != null) {
+ adder.increment();
+ return;
+ }
LongAdder item = inboundDistribution.computeIfAbsent(requestCode, k -> new LongAdder());
+ lastInAdder = item;
+ lastInCode = requestCode;
item.increment();
Review Comment:
Good catch. Fixed by introducing an immutable `CachedCounter { final int code; final LongAdder adder; }` holder published via a single volatile write. Reading threads now load one volatile reference and always observe a consistent pair. The worst-case race is a redundant `computeIfAbsent` (no correctness issue).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org