[ISSUE #10527] Reduce auxiliary component allocation via AtomicLong, string caches, StringBuilder reuse, and dirty flag

[wang-jiahua]

Which Issue(s) This PR Fixes

Fixes #10527

Brief Description

Reduce allocation in auxiliary store components through four independent optimizations:

1. QueueOffsetOperator — Replace ConcurrentMap<String, Long> with ConcurrentMap<String, AtomicLong>. Eliminates Long boxing on every queue offset update.
2. BrokerStatsManager — Change Integer parameters to int in incQueue* methods to eliminate autoboxing. (String caching was removed after microbenchmark showed ConcurrentHashMap lookup is slower than JIT-optimized String concat.)
3. IndexService — Reuse StringBuilder via member field instead of allocating per-call.
4. TimerWheel — Add volatile dirty flag to skip flush when no changes occurred since last flush.
5. AppendMessageResult — Add constructor with pre-computed fields to avoid redundant allocation.

How Did You Test This Change?

1. Unit tests: All 15 BrokerStatsManagerTest tests pass.
2. Compilation: store module compiles cleanly on JDK 21.
3. Commercial compatibility: No commercial classes extend any modified class.
4. Microbenchmark: QueueOffsetOperator AtomicLong is 46% faster than Long boxing. String caching was removed because microbenchmark showed it was 120% slower than JIT-optimized String concatenation (ConcurrentHashMap lookup overhead exceeds String allocation cost for short strings).

[Copilot]

Pull request overview

Note

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

This PR focuses on reducing allocation/flush overhead in the store layer by introducing cached key builders, adding a “dirty” optimization for timer wheel flushing, and improving queue offset updates with atomic counters.

Changes:

• Add key caching and primitive int queueId APIs in BrokerStatsManager to reduce stats-key allocation.
• Optimize TimerWheel.flush() using a dirty flag and 8-byte aligned comparisons to reduce unnecessary IO/copying.
• Replace queue offset maps from Long values to AtomicLong for safer concurrent increments; add key-building reuse in IndexService.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 17 comments.

Show a summary per file

File	Description
store/src/test/java/org/apache/rocketmq/store/stats/BrokerStatsManagerTest.java	Align test constant type (Integer → int) with production API changes.
store/src/main/java/org/apache/rocketmq/store/timer/TimerWheel.java	Add dirty gating and faster flush loop to reduce forced writes.
store/src/main/java/org/apache/rocketmq/store/stats/BrokerStatsManager.java	Add multiple caches for stats key strings and switch queueId params to primitive int.
store/src/main/java/org/apache/rocketmq/store/queue/QueueOffsetOperator.java	Use AtomicLong to avoid lost updates on concurrent offset increments; add snapshot/convert helpers.
store/src/main/java/org/apache/rocketmq/store/index/IndexService.java	Reuse a StringBuilder for key construction to reduce temporary allocations.
store/src/main/java/org/apache/rocketmq/store/AppendMessageResult.java	Add an additional constructor overload mirroring existing fields.

---

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

[RockteMQ-AI]

Review by github-manager-bot

Summary

This PR reduces auxiliary component allocation in the store layer through five optimization strategies: reusable StringBuilder in IndexService, AtomicLong-based offset tracking in QueueOffsetOperator, string caching in BrokerStatsManager, dirty-flag flush gating in TimerWheel, and a new AppendMessageResult constructor overload. Overall direction is sound — reducing per-message allocation pressure in hot paths.

Findings

• [Critical] IndexService.java:52 — Thread-safety violation with reusableKeyBuilder. IndexService is shared across threads (commitlog dispatch, query, etc.), and buildKey() is called from both queryOffset() (read lock) and buildIndex() (write lock). A shared mutable StringBuilder without synchronization will produce corrupted keys under concurrent access. The JVM may even reorder setLength(0) and append() calls across threads. This is a data corruption risk — keys will be silently wrong, causing index lookups to miss or return incorrect offsets.

  • Fix: Either use a ThreadLocal<StringBuilder>, pass the builder as a parameter from callers that already hold appropriate locks, or revert to the original string concatenation (the JIT will optimize topic + "#" + key into a StringBuilder anyway, and the allocation is short-lived).

• [Critical] TimerWheel.java:58,137,151,303,313,326,332 — dirty flag race condition. The pattern if (!dirty) return; ... dirty = false; is a non-atomic check-then-act. Between the if (!dirty) guard and the final dirty = false, another thread calling setDirty(true) can have its update lost — the flush proceeds but then resets the flag, losing the knowledge that new data was written. Over time this can cause the timer wheel to skip flushes, leading to data loss on crash.

  • Fix: Use AtomicBoolean with compareAndSet(true, false) to atomically claim the dirty state, or synchronize the flush method. Alternatively, accept that some flushes may be redundant (safe) by checking dirty again after the flush.

• [Warning] BrokerStatsManager.java:496 — Unbounded cache growth. The statsKeyByGroupCache and similar ConcurrentHashMap<String, String[]> caches grow without eviction. In deployments with thousands of topics × groups, this can accumulate significant heap. Consider adding a size cap or using a bounded cache (e.g., Caffeine with maximumSize).

• [Info] QueueOffsetOperator.java — The change from Long to AtomicLong for offset values is a good correctness improvement (atomic increment vs. non-atomic read-modify-write). The getTopicQueueTable() snapshot copy is appropriate for iteration safety.

Suggestions

1. The IndexService thread-safety issue is a blocker — please fix before merge.
2. The TimerWheel dirty flag issue should also be addressed; consider AtomicBoolean.getAndSet(false) as a minimal fix.
3. Consider adding a JMH benchmark for the hot paths to quantify the allocation reduction.

---

Automated review by github-manager-bot

[codecov-commenter]

Codecov Report

❌ Patch coverage is 64.51613% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.09%. Comparing base (59a70d9) to head (f2503ca).
⚠️ Report is 6 commits behind head on develop.

Files with missing lines	Patch %	Lines
...org/apache/rocketmq/store/AppendMessageResult.java	0.00%	10 Missing ⚠️
...ache/rocketmq/store/queue/QueueOffsetOperator.java	74.19%	7 Missing and 1 partial ⚠️
...va/org/apache/rocketmq/store/timer/TimerWheel.java	75.00%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10528      +/-   ##
=============================================
- Coverage      48.19%   48.09%   -0.11%     
+ Complexity     13398    13382      -16     
=============================================
  Files           1377     1377              
  Lines         100730   100876     +146     
  Branches       13012    13041      +29     
=============================================
- Hits           48548    48516      -32     
- Misses         46252    46404     +152     
- Partials        5930     5956      +26     

☔ 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.

[RockteMQ-AI]

Review by github-manager-bot (Re-review after new commits)

Summary

Re-review after new commit f2503ca0 (2026-06-19). The update adds a new AppendMessageResult constructor overload and refines QueueOffsetOperator to use AtomicLong for offset tracking. However, the two critical issues from the previous review remain unaddressed.

Unresolved Critical Issues

• [Critical] IndexService.java — reusableKeyBuilder thread-safety violation persists. The shared StringBuilder instance field is still used in buildKey() without synchronization. IndexService is accessed from multiple threads (dispatch, query). Under concurrent access, setLength(0) + append() interleaving will produce corrupted keys, causing silent index lookup failures.

  • Fix: Use ThreadLocal<StringBuilder>, or revert to topic + "#" + key (JIT optimizes this).

• [Critical] TimerWheel.java — dirty flag race condition persists. The if (!dirty) return; ... dirty = false; pattern is a non-atomic check-then-act. Between the guard and the reset, another thread can set dirty = true which then gets silently cleared, causing missed flushes and potential data loss on crash.

  • Fix: Use AtomicBoolean.compareAndSet(true, false) to atomically claim the dirty state.

New Changes

• [Info] AppendMessageResult.java — New 8-arg constructor (drops queueOffset and logicsOffset). Callers that don't need these fields can use this lighter constructor. No issues.

• [Info] QueueOffsetOperator.java — AtomicLong-based offset tracking is a good correctness improvement. The getTopicQueueTable() snapshot copy is appropriate for iteration safety. The setTopicQueueTable conversion from Map<String, Long> to Map<String, AtomicLong> is a one-time cost during load, acceptable.

• [Info] BrokerStatsManagerTest.java — Integer QUEUE_ID → int QUEUE_ID avoids autoboxing. Minor but correct.

Action Required

The two critical thread-safety issues must be resolved before merge. Please fix IndexService and TimerWheel as suggested.

---

Automated re-review by github-manager-bot