qianye1001 commented on issue #10400:
URL: https://github.com/apache/rocketmq/issues/10400#issuecomment-4561605609
## 🤖 Automated Fix Proposal (v1)
I've analyzed this enhancement against the latest upstream code and
generated a fix specification.
**Code Base:** c9e51d625e1d (develop)
### Summary
**Problem:** `Version.values()` is called **6 times** across
`getVersionDesc()` and `value2Version()` in `MQVersion.java`. Each call
allocates a new defensive copy of a **943-element array** (~7.5 KB). These
methods are on hot paths (broker heartbeat, client connection, protocol
negotiation) called from **14 files** across the codebase.
**Fix Strategy:**
1. Add `private static final Version[] VERSION_VALUES = Version.values();` —
cache the array once at class load
2. Rewrite both methods to use a local reference to the cached array
3. No API changes, no behavioral changes — pure internal optimization
**File to Modify:**
- `common/src/main/java/org/apache/rocketmq/common/MQVersion.java` — add
cached field + rewrite 2 methods
**Risk Assessment:** Very Low — textbook enum caching optimization, fully
backward compatible, no new dependencies.
<details>
<summary>Full Specification (click to expand)</summary>
```markdown
# Fix Spec: Cache Version.values() in MQVersion
**Issue:** #10400 - [Enhancement] Cache Version.values() in MQVersion to
avoid repeated array allocation
**Status:** Enhancement verified
**File:** `common/src/main/java/org/apache/rocketmq/common/MQVersion.java`
---
## 1. Root Cause Analysis
`Version.values()` is a compiler-generated method on Java enums that returns
a **defensive copy** of the internal constant array on every invocation. In
`MQVersion.java`, this method is called **6 times** across two hot-path utility
methods (`getVersionDesc` and `value2Version`):
- Lines 24, 26, 29 in `getVersionDesc(int value)`
- Lines 33, 35, 38 in `value2Version(int value)`
The `Version` enum contains **943 constants**. Each `values()` call
allocates a new `Version[943]` array and copies all references into it. Since
`getVersionDesc` and `value2Version` are called on every broker heartbeat,
client connection, and protocol negotiation, this causes:
1. **Unnecessary heap allocation** (~7.5 KB per call at 8 bytes/reference)
2. **GC pressure** from short-lived arrays in high-throughput scenarios
3. **CPU waste** from repeated `System.arraycopy` inside `values()`
A single method invocation of `getVersionDesc` triggers **3 separate array
allocations** (lines 24, 26/29), all of which are immediately discarded.
---
## 2. Fix Strategy
### Step 1: Add a cached static array field
Add a `private static final Version[]` field initialized once at class load
time:
```java
private static final Version[] VERSION_VALUES = Version.values();
```
This is safe because:
- Enum constants are immutable singletons
- The array content is never modified (we enforce this by keeping the field
`private`)
- Class loading guarantees thread-safe initialization
### Step 2: Rewrite `getVersionDesc()` using cached array
Replace all 3 `Version.values()` calls with a local variable referencing the
cached array:
```java
public static String getVersionDesc(int value) {
Version[] versions = VERSION_VALUES;
int length = versions.length;
if (value >= length) {
return versions[length - 1].name();
}
return versions[value].name();
}
```
Using a local variable (`versions`) avoids repeated field access and
prevents external code from holding a reference to the internal array.
### Step 3: Rewrite `value2Version()` using cached array
Same pattern:
```java
public static Version value2Version(int value) {
Version[] versions = VERSION_VALUES;
int length = versions.length;
if (value >= length) {
return versions[length - 1];
}
return versions[value];
}
```
---
## 3. Files to Modify
### `common/src/main/java/org/apache/rocketmq/common/MQVersion.java`
| Line(s) | Change |
|----------|--------|
| After line 21 | Add: `private static final Version[] VERSION_VALUES =
Version.values();` |
| Lines 23-30 | Rewrite `getVersionDesc()` to use local ref to
`VERSION_VALUES` |
| Lines 32-39 | Rewrite `value2Version()` to use local ref to
`VERSION_VALUES` |
**No other files require modification.** The public API signatures of
`getVersionDesc(int)` and `value2Version(int)` remain unchanged.
---
## 4. Test Plan
### 4.1 Existing Tests
Run existing unit tests to confirm no regression:
```bash
mvn test -pl common -Dtest=MQVersionTest
```
### 4.2 Behavioral Verification
Verify the following scenarios still produce correct results:
| Input | Expected `getVersionDesc` | Expected `value2Version` |
|-------|---------------------------|--------------------------|
| `0` | `"V3_0_0_SNAPSHOT"` | `Version.V3_0_0_SNAPSHOT` |
| `Version.V5_5_0.ordinal()` | `"V5_5_0"` | `Version.V5_5_0` |
| `Integer.MAX_VALUE` (out of range) | `"HIGHER_VERSION"` |
`Version.HIGHER_VERSION` |
| Negative value | Should throw `ArrayIndexOutOfBoundsException` (existing
behavior preserved) |
### 4.3 Concurrency Safety
No additional synchronization tests needed: the cached array is `static
final` and initialized during class loading (JLS 12.4.2 guarantees thread
safety).
---
## 5. Backward Compatibility
| Aspect | Impact |
|--------|--------|
| Public API | **No change** - method signatures identical |
| Behavior | **Identical** - same values returned for same inputs |
| Binary compatibility | **Full** - no changes to class/method descriptors |
| Serialization | **N/A** - no serializable state affected |
| Thread safety | **Improved** - eliminates per-call allocation race window |
The fix is a **pure internal optimization** with zero externally observable
behavioral change.
---
## 6. Risk Assessment
| Risk | Likelihood | Impact | Mitigation |
|------|-----------|--------|------------|
| Array mutation by reflection | Very Low | Medium | Field is `private
static final`; standard Java encapsulation. Malicious reflection is out of
scope. |
| Enum constant addition at runtime | None | N/A | Java enums are
compile-time fixed. New versions require recompilation. |
| Memory leak from pinned array | None | N/A | Single 943-element array
(~7.5 KB) lives for JVM lifetime - negligible. |
| Incorrect ordinal mapping | Very Low | High | Existing tests cover
ordinal-to-version mapping. No logic change in mapping. |
**Overall Risk: Very Low.** This is a textbook enum caching optimization
with no behavioral change.
```
</details>
### Next Steps
- Reply `/approve` to proceed with generating a PR
- Reply `/revise <feedback>` to request changes to the fix approach
- Reply `/reject` to close this proposal
*This proposal will expire in 72 hours if no response is received.*
--
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]
