[ISSUE #10512] Eliminate per-RPC allocation in RemotingCommand (Guava Stopwatch, Constructor copy) and downgrade Netty writability log

[wang-jiahua]

Summary

Four independent per-RPC micro-allocations eliminated in the remoting framework:

1. Guava Stopwatch → System.nanoTime()

RemotingCommand used Stopwatch.createStarted() which allocates a new object per RPC. Replaced with long processTimerNanos + processTimerElapsedMs().

2. Constructor cache

Class.getDeclaredConstructor() copies the Constructor object on every call (~237 allocation events per 60s JFR). Added ConcurrentHashMap<Class<?>, Constructor<?>> HEADER_CTOR_CACHE with computeIfAbsent to pay the reflective lookup once per class.

3. Netty writability log downgrade

NettyRemotingServer.channelWritabilityChanged logged at INFO/WARN on every writability change (~81,434 lines in 90s = ~900 lines/sec). Downgraded to DEBUG with isDebugEnabled() guard.

4. TopicQueueMappingContext.EMPTY singleton

Non-static-topic messages (>99%) created empty TopicQueueMappingContext objects. Added public static final EMPTY singleton.

Files Changed

File	Change
remoting/.../protocol/RemotingCommand.java	+56/-10: Stopwatch→nanoTime + Constructor cache + newHeaderInstance() + setProcessTimerNanos()/processTimerElapsedMs()
remoting/.../netty/NettyDecoder.java	+2/-3: Stopwatch → System.nanoTime() + setProcessTimerNanos()
remoting/.../netty/NettyRemotingServer.java	+8/-4: channelWritabilityChanged INFO/WARN → DEBUG with isDebugEnabled() guard
remoting/.../protocol/statictopic/TopicQueueMappingContext.java	+2: EMPTY static singleton

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR optimizes remoting command processing by reducing allocation overhead (Stopwatch removal, constructor caching, smaller HashMap defaults) and adds a faster header encoding path while also lowering channel writability log verbosity.

Changes:

• Replace Guava Stopwatch timing with a System.nanoTime()-based timestamp on RemotingCommand.
• Cache CommandCustomHeader no-arg constructors and add fastEncodeHeaderAsBuffer.
• Reduce per-command allocations (smaller extFields default capacity) and downgrade some writability logs to debug.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File	Description
remoting/src/main/java/org/apache/rocketmq/remoting/protocol/statictopic/TopicQueueMappingContext.java	Adds a shared EMPTY instance for “no context” usage.
remoting/src/main/java/org/apache/rocketmq/remoting/protocol/RemotingCommand.java	Constructor caching, new fast header encoding method, Stopwatch removal, extFields capacity change.
remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingServer.java	Lowers channel writability logs to debug and guards formatting work.
remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyDecoder.java	Switches decode timing from Stopwatch to nanoTime start timestamp.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[RockteMQ-AI]

Review by github-manager-bot

Summary

Eliminates per-RPC allocations in the remoting framework: replaces Guava Stopwatch with System.nanoTime(), caches Constructor lookups, downgrades Netty writability log to DEBUG, and adds TopicQueueMappingContext.EMPTY singleton.

Findings

• [Info] RemotingCommand.java — HEADER_CTOR_CACHE uses ConcurrentHashMap.computeIfAbsent(). The mapping function calls ctor.newInstance() which is reflective and could throw. Consider wrapping the computeIfAbsent lambda with a try-catch to avoid swallowing checked exceptions silently, or use get() with a null check + putIfAbsent() pattern for clearer error propagation.
• [Info] RemotingCommand.java — processTimerNanos defaults to 0. If setProcessTimerNanos() is never called (e.g., code paths that bypass NettyDecoder), processTimerElapsedMs() returns 0. This is likely intentional but worth a brief comment documenting the contract.
• [Info] NettyRemotingServer.java — Good catch on the log amplification (~900 lines/sec). The isDebugEnabled() guard is correct and avoids unnecessary string formatting.

Suggestions

• Consider adding a unit test for newHeaderInstance() to verify the constructor cache works correctly for subclasses with different constructor signatures.
• The EMPTY singleton in TopicQueueMappingContext is a good optimization. Ensure no code path mutates the singleton (e.g., call setTopic() on it). If immutability cannot be guaranteed, consider returning a defensive copy or making the fields final.

Overall: Clean micro-optimizations with measurable impact on high-throughput paths. 👍

---

Automated review by github-manager-bot

[RockteMQ-AI]

Review by github-manager-bot

Summary

Four independent per-RPC micro-optimizations in the remoting framework:

1. Guava Stopwatch → System.nanoTime() (eliminates per-RPC allocation)
2. Constructor cache for Class.getDeclaredConstructor() (eliminates ~237 alloc events/60s)
3. Netty writability log downgrade INFO/WARN → DEBUG (eliminates ~900 lines/sec)
4. TopicQueueMappingContext.EMPTY singleton (avoids per-message allocation for non-static-topic messages)

Findings

• [Positive] RemotingCommand.java — Replacing Stopwatch with raw long processTimerNanos eliminates one object allocation per RPC. The newHeaderInstance() method with ConcurrentHashMap constructor cache is a clean solution to the getDeclaredConstructor() defensive copy issue.
• [Positive] NettyRemotingServer.java — Downgrading channelWritabilityChanged to DEBUG with isDebugEnabled() guard is appropriate. ~900 lines/sec of INFO/WARN is clearly excessive for a writability toggle event.
• [Positive] TopicQueueMappingContext.java — Adding EMPTY singleton is a simple and effective optimization.
• [Warning] RemotingCommand.java — The HEADER_CTOR_CACHE uses computeIfAbsent on a ConcurrentHashMap. Under high contention with many distinct header classes, this could cause brief blocking on the bin lock. However, in practice the number of distinct header classes is bounded and small, so this is acceptable.
• [Warning] RemotingCommand.java — The processTimerElapsedMs() method computes (System.nanoTime() - processTimerNanos) / 1_000_000. Note that nanoTime() can overflow after ~292 years of JVM uptime, and the subtraction handles this correctly due to two's complement arithmetic. No issue here, just noting for completeness.
• [Info] NettyDecoder.java — Clean removal of the Guava Stopwatch import. The long timerNanos = System.nanoTime() is a drop-in replacement.

Suggestions

• Consider adding a brief comment on HEADER_CTOR_CACHE explaining why a ConcurrentHashMap was chosen over ClassValue (which is specifically designed for per-class lazy computation). ClassValue would avoid the explicit cache map and may have better GC characteristics.
• The four changes are independent — if the maintainers prefer smaller PRs, these could be split. However, they are all in the same hot path and the PR description clearly separates them, so keeping them together is also reasonable.

Verdict

LGTM. Well-justified micro-optimizations with clear JFR evidence.

---

Automated review by github-manager-bot

[RockteMQ-AI]

Review by github-manager-bot (re-review)

Summary

Four independent per-RPC micro-optimizations in the remoting framework:

1. Guava Stopwatch → System.nanoTime() (eliminates per-RPC object allocation)
2. Constructor cache for Class.getDeclaredConstructor() (eliminates ~237 alloc events/60s in JFR)
3. Netty writability log downgrade INFO/WARN → DEBUG (eliminates ~900 lines/sec)
4. TopicQueueMappingContext.EMPTY singleton (avoids per-message allocation for >99% of messages)

Findings

• [Positive] RemotingCommand.java — Replacing Stopwatch with a raw long processTimerNanos + processTimerElapsedMs() is a clean elimination of per-RPC allocation. The accessor method encapsulates the conversion well.
• [Positive] RemotingCommand.java — HEADER_CTOR_CACHE with ConcurrentHashMap.computeIfAbsent() correctly avoids the per-call Constructor copy. The newHeaderInstance() helper centralizes the reflective construction with proper error handling.
• [Positive] NettyRemotingServer.java — Downgrading channelWritabilityChanged to DEBUG with isDebugEnabled() guard is the right call. ~900 lines/sec at INFO was clearly excessive.
• [Info] TopicQueueMappingContext.java — The EMPTY singleton is a simple and effective optimization. Ensure callers don't mutate it — consider making fields immutable or adding a comment documenting the shared-instance contract.
• [Info] Cross-repo: RemotingCommand changes (especially setProcessTimerNanos / processTimerElapsedMs) touch the core remoting API. If apache/rocketmq-clients Java module depends on RemotingCommand, verify no breakage there.

Verdict

Solid set of micro-optimizations with clear JFR evidence. No correctness concerns.

---

Automated review by github-manager-bot

[RockteMQ-AI]

Review by github-manager-bot (re-review after new commit)

Summary

Same four per-RPC micro-optimizations as previously reviewed. The new commit squashes the changes into a single commit without altering the diff content.

Verdict

No changes since the previous review. All findings remain valid. Approving.

LGTM ✅

---

Automated review by github-manager-bot

[codecov-commenter]

Codecov Report

❌ Patch coverage is 26.00000% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.15%. Comparing base (59a70d9) to head (3a84cc7).

Files with missing lines	Patch %	Lines
...he/rocketmq/remoting/protocol/RemotingCommand.java	39.28%	17 Missing ⚠️
...rocketmq/remoting/netty/NettyRemotingAbstract.java	0.00%	7 Missing ⚠️
...e/rocketmq/remoting/netty/NettyRemotingServer.java	0.00%	6 Missing ⚠️
...rocketmq/broker/processor/PopMessageProcessor.java	0.00%	2 Missing ⚠️
...cketmq/broker/processor/QueryMessageProcessor.java	0.00%	2 Missing ⚠️
...ker/processor/DefaultPullMessageResultHandler.java	0.00%	1 Missing ⚠️
...ocketmq/broker/processor/PeekMessageProcessor.java	0.00%	1 Missing ⚠️
...protocol/statictopic/TopicQueueMappingContext.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10514      +/-   ##
=============================================
- Coverage      48.17%   48.15%   -0.02%     
+ Complexity     13394    13392       -2     
=============================================
  Files           1377     1377              
  Lines         100730   100755      +25     
  Branches       13012    13017       +5     
=============================================
- Hits           48530    48522       -8     
- Misses         46264    46283      +19     
- Partials        5936     5950      +14     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.