Skip to content

[ISSUE #10400] Cache Version.values() in MQVersion to avoid repeated array allocation#10401

Open
qianye1001 wants to merge 2 commits into
apache:developfrom
qianye1001:fix/issue-10400
Open

[ISSUE #10400] Cache Version.values() in MQVersion to avoid repeated array allocation#10401
qianye1001 wants to merge 2 commits into
apache:developfrom
qianye1001:fix/issue-10400

Conversation

@qianye1001
Copy link
Copy Markdown
Contributor

Summary

Cache Version.values() in a static final field to eliminate repeated defensive-copy array allocations in hot-path methods getVersionDesc() and value2Version().

Problem

Version.values() returns a defensive copy of a 943-element enum array on every call. These two methods call it 6 times total, allocating ~7.5KB of heap per invocation. Under high throughput (broker heartbeats, client connections), this creates unnecessary GC pressure.

Fix

  • Added private static final Version[] VERSION_VALUES = Version.values(); (initialized once at class load)
  • Rewrote both methods to use a local reference to the cached array
  • Public API signatures unchanged — pure internal optimization, zero behavioral change

Changes

File Change
common/.../MQVersion.java Cache enum array, rewrite 2 methods

Related Issue

Closes #10400


This PR was automatically generated.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 47.92%. Comparing base (a705dbc) to head (5dd98cc).

Additional details and impacted files
@@            Coverage Diff             @@
##             develop   #10401   +/-   ##
==========================================
  Coverage      47.91%   47.92%           
- Complexity     13248    13250    +2     
==========================================
  Files           1376     1376           
  Lines         100536   100539    +3     
  Branches       12983    12983           
==========================================
+ Hits           48172    48181    +9     
+ Misses         46424    46419    -5     
+ Partials        5940     5939    -1     

☔ View full report in Codecov by Sentry.
📢 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.

Copy link
Copy Markdown
Contributor Author

@qianye1001 qianye1001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by rocketmq-reviewer-bot

Summary

Cache Version.values() result in a private static final field to eliminate repeated defensive-copy array allocations (~7.5KB per call) in getVersionDesc() and value2Version() hot-path methods.

Findings

  • [Info] MQVersion.java:23private static final Version[] VERSION_VALUES = Version.values(); is initialized once at class loading time. The enum array is immutable after JVM startup (no new enum constants can be added at runtime), so caching is safe. ✅

  • [Info] Both methods — Using a local variable Version[] versions = VERSION_VALUES; before accessing .length and indexing is a good practice. It avoids repeated field reads and is JIT-friendly. ✅

  • [Low] MQVersion.java:23 — The cached array is mutable (Java arrays are always mutable). Since VERSION_VALUES is private and only used read-only within this class, the risk is negligible. However, if any future code were to modify the array (e.g., VERSION_VALUES[0] = null), it would corrupt all lookups. This is a very low risk given the current usage, but worth noting for future maintainers.

  • [Suggestion] Consider adding a brief comment on the cached field explaining why Version.values() is cached (defensive copy avoidance) — this helps future developers understand the optimization intent without needing to trace the git history.

  • [Info] The PR description mentions 943 enum elements and ~7.5KB per allocation. This is a meaningful optimization for high-throughput broker scenarios (heartbeats, client connections). The fix is minimal, well-isolated, and follows a standard Java enum caching pattern.

Cross-repo Note

No cross-repo impact. MQVersion is a common utility class; the change is purely internal performance optimization with no API or behavioral changes.

Verdict

Approve — Clean, minimal performance optimization. The "cache values() in static final + use local variable" pattern is a well-established Java idiom for enum-heavy hot paths. No API changes, no compatibility risk, no cross-repo impact. The only suggestion is to add a brief comment on the field for future maintainability.


Automated review by rocketmq-reviewer-bot

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Enhancement] Cache Version.values() in MQVersion to avoid repeated array allocation

4 participants