RockteMQ-AI commented on PR #10514: URL: https://github.com/apache/rocketmq/pull/10514#issuecomment-4976328993
## Review by github-manager-bot ### Summary Re-review after rebase (commit `b6dcf7e0`, 2026-07-14). This PR eliminates four categories of per-RPC micro-allocations in the remoting framework. 9 files changed (+100/-32). ### Findings - **[Info] Stopwatch → System.nanoTime()** — Replacing `Stopwatch.createStarted()` with a raw `long processTimerNanos` eliminates one object allocation per RPC. The `processTimerElapsedMs()` convenience method provides a clean abstraction. All 4 broker processor call sites (`DefaultPullMessageResultHandler`, `PeekMessageProcessor`, `PopMessageProcessor`, `QueryMessageProcessor`) are consistently updated. ✓ - **[Info] Deprecated compatibility methods** — `getProcessTimer()` is preserved as a `@Deprecated` method returning a fake `Stopwatch` wrapping the stored nanos. This is a good backward-compatibility strategy for any external code (e.g., commercial extensions) that calls `getProcessTimer()`. ✓ - **[Info] Constructor cache (HEADER_CTOR_CACHE)** — `ConcurrentHashMap` with `get()` + `putIfAbsent()` pattern. Thread-safe: multiple threads may create a `Constructor` simultaneously, but `putIfAbsent` ensures only one wins. The `setAccessible(true)` call is on the local reference before insertion, which is correct. ✓ - **[Info] Netty writability log downgrade** — `channelWritabilityChanged` changed from INFO/WARN to DEBUG with `isDebugEnabled()` guard. This is appropriate — writability toggling under load generates excessive log volume at WARN level. ✓ - **[Info] addExtField capacity reduction** — Default capacity changed from 256 to 16 (HashMap default). Most RPC commands have few ext fields, so 256 was significantly over-allocated. Good optimization. ✓ - **[Warning] TopicQueueMappingContext change** — The PR description mentions adding a `public static final EMPTY` singleton, but the diff only shows a blank line addition to `TopicQueueMappingContext.java`. If the EMPTY singleton is planned, it should be included in this commit; if it was already merged elsewhere, the description should be updated. - **[Info] Test coverage** — `RemotingCommandTest` adds tests for the deprecated `getProcessTimer()` compatibility path. Good. ### Suggestions 1. Clarify the `TopicQueueMappingContext` change — the diff does not match the PR description's EMPTY singleton claim. 2. Consider adding a microbenchmark for the constructor cache hit path to quantify the allocation savings. --- *Automated review by github-manager-bot* -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
