[ISSUE #10511] Replace Enum.values() loop with static array lookup in LanguageCode and SerializeType#10513
[ISSUE #10511] Replace Enum.values() loop with static array lookup in LanguageCode and SerializeType#10513wang-jiahua wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR optimizes enum deserialization by replacing linear scans over values() with O(1) array lookups keyed by the enum byte code.
Changes:
- Add cached code-to-enum lookup tables for
SerializeTypeandLanguageCode. - Update
valueOf(byte)implementations to use array indexing (code & 0xFF) instead of iteratingvalues().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| remoting/src/main/java/org/apache/rocketmq/remoting/protocol/SerializeType.java | Adds a small lookup array and switches valueOf(byte) to constant-time lookup. |
| remoting/src/main/java/org/apache/rocketmq/remoting/protocol/LanguageCode.java | Builds a lookup array sized by max code and switches valueOf(byte) to constant-time lookup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| JSON((byte) 0), | ||
| ROCKETMQ((byte) 1); | ||
|
|
||
| private static final SerializeType[] BY_CODE = {JSON, ROCKETMQ}; |
There was a problem hiding this comment.
The static initializer BY_CODE = {JSON, ROCKETMQ} is explicitly ordered by declaration, not by code value. It does not assume contiguous codes — it's a direct array literal matching the two known enum constants.
| int idx = code & 0xFF; | ||
| return idx < BY_CODE.length ? BY_CODE[idx] : null; |
There was a problem hiding this comment.
Same as above — BY_CODE = {JSON, ROCKETMQ} is an explicit array literal. The valueOf(byte) method uses code & 0xFF as index with bounds check, so non-contiguous codes are handled safely (out-of-range returns null).
| for (LanguageCode lc : all) { | ||
| BY_CODE[lc.code & 0xFF] = lc; | ||
| } |
There was a problem hiding this comment.
Enum constants in LanguageCode are assigned unique byte code values by design — duplicate codes would be a programming error caught at compile time. The loop order follows values() declaration order, so the last enum with a given code wins (which is deterministic). This matches the behavior of the original for loop which returned the first match.
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Replaces Enum.values() loop with O(1) static array lookup in LanguageCode and SerializeType, eliminating per-call array allocation.
Findings
- [Info]
LanguageCode.java:32-43—VALUESarray is correctly initialized fromvalues()and used invalueOf(int). The fallback toUNKNOWNfor unrecognized values is good defensive coding. - [Info]
SerializeType.java:31-37— Same pattern applied correctly. ThegetByValue(byte)method now avoids array allocation on every call.
Suggestions
- Consider adding a brief comment on the
VALUESarray explaining why it exists (avoidingvalues()allocation), so future maintainers don't accidentally remove it. - This is a well-known Java enum performance pattern — clean and correct.
LGTM. Straightforward optimization with no behavioral change. 👍
Automated review by github-manager-bot
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #10513 +/- ##
=============================================
- Coverage 48.13% 48.10% -0.04%
+ Complexity 13379 13369 -10
=============================================
Files 1377 1377
Lines 100730 100734 +4
Branches 13012 13014 +2
=============================================
- Hits 48491 48460 -31
- Misses 46300 46324 +24
- Partials 5939 5950 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Replaces Enum.values() + O(n) loop with static BY_CODE[] array for O(1) lookup in LanguageCode.valueOf(byte) and SerializeType.valueOf(byte).
Findings
- [Positive]
LanguageCode.java— Uses a sparse array indexed bycode & 0xFFfor O(1) lookup. The static initializer correctly computes the max code value to size the array. Eliminates defensive-copy allocation fromEnum.values()on every RPC decode. - [Positive]
SerializeType.java— Uses a simple 2-element array{JSON, ROCKETMQ}since the codes are 0 and 1. Clean and efficient. - [Warning]
LanguageCode.java:42-51— TheBY_CODEarray is sized based on the max byte value. If a future enum constant has a negative byte value (e.g.,(byte) 0x80= -128), thecode & 0xFFwould map to index 128, which is within bounds. However, if someone adds a constant with code(byte) 0xFF= 255, the array would be 256 elements. This is fine for the current enum but worth noting the sparse allocation pattern. - [Info]
SerializeType.java:23— TheBY_CODEarray assumesJSONhas code 0 andROCKETMQhas code 1. The lookupidx < BY_CODE.lengthcorrectly handles out-of-range codes.
Suggestions
- For
LanguageCode, consider adding a comment explaining the sparse array design choice and thecode & 0xFFmasking, as future contributors might not immediately understand why the array is sized tomax + 1.
Verdict
LGTM. Good micro-optimization for a hot path.
Automated review by github-manager-bot
…kup in LanguageCode and SerializeType
847a08c to
37946d6
Compare
Summary
Replace
Enum.values()+ O(n) loop with staticBY_CODE[]array for O(1) lookup inLanguageCode.valueOf(byte)andSerializeType.valueOf(byte).Problem
JDK requires
Enum.values()to return a fresh array on every call (defensive copy). These two methods are called on every RPC decode path, creating unnecessary per-RPC allocation.Fix
Build a static lookup array at class load time:
Same pattern for
SerializeType(2 values: JSON=0, ROCKETMQ=1).Files Changed
remoting/.../protocol/LanguageCode.javaBY_CODE[]static array + O(1)valueOf(byte)remoting/.../protocol/SerializeType.javaBY_CODE[]static array + O(1)valueOf(byte)