[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.19%. Comparing base (88709c5) to head (bee9c4a).

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.28%   48.19%   -0.10%     
+ Complexity     13445    13414      -31     
=============================================
  Files           1378     1378              
  Lines         100845   100883      +38     
  Branches       13044    13051       +7     
=============================================
- Hits           48695    48617      -78     
- Misses         46201    46294      +93     
- Partials        5949     5972      +23     

☔ 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

[wang-jiahua]

Server Benchmark Results (2026-06-22)

Environment: 8C30G broker, Dragonwell JDK 21, 1KB msg, 128 queues, sync mode, 256 threads, 20s warmup + 30s collect

Producer-only (steady state, lines 3-5)

Test	TPS	Avg RT	GC	vs Baseline
Baseline	217k	1.18ms	354	—
#10528	224k	1.14ms	332	+3.2% TPS, -3.4% RT ✅

Auxiliary component optimizations (QueueOffsetOperator AtomicLong, IndexService StringBuilder reuse, TimerWheel dirty flag) reduce boxing and string allocation on the put message path.

[RockteMQ-AI]

Review by github-manager-bot

Summary

Reduces auxiliary component allocation via AtomicLong counters, cached string keys, StringBuilder reuse, and dirty flag patterns.

Issues Found

[Critical] IndexService.reusableKeyBuilder is not thread-safe

private final StringBuilder reusableKeyBuilder = new StringBuilder(128);

private String buildKey(final String topic, final String key) {
    reusableKeyBuilder.setLength(0);
    return reusableKeyBuilder.append(topic).append(\"#\").append(key).toString();
}

IndexService is a shared service used by multiple threads (dispatch threads, query threads). buildKey() is called from queryOffset() and buildIndex() which can execute concurrently. StringBuilder is not thread-safe — concurrent setLength(0) + append() calls will produce corrupted keys or throw StringIndexOutOfBoundsException.

Suggested fix: Use ThreadLocal<StringBuilder> instead:

private static final ThreadLocal<StringBuilder> KEY_BUILDER = ThreadLocal.withInitial(() -> new StringBuilder(128));

private String buildKey(final String topic, final String key) {
    StringBuilder sb = KEY_BUILDER.get();
    sb.setLength(0);
    return sb.append(topic).append(\"#\").append(key).toString();
}

Other Findings

• [Info] QueueOffsetOperator AtomicLong migration — Clean. Eliminates boxing/unboxing on every offset update. The ConcurrentHashMap<String, AtomicLong> pattern is correct.
• [Info] AppendMessageResult new constructor — Good, avoids unused StringBuilder parameter.
• [Info] ConsumeQueue.topicQueueKey caching — Good optimization, avoids repeated string concatenation.

---

Automated review by github-manager-bot