[ISSUE #10973] Reduce allocation in ExtraInfoUtil parsing and primitive getters - #10975
[ISSUE #10973] Reduce allocation in ExtraInfoUtil parsing and primitive getters#10975wang-jiahua wants to merge 1 commit into
Conversation
…rimitive getters
There was a problem hiding this comment.
Pull request overview
This PR optimizes the POP/ACK hot path in ExtraInfoUtil by reducing allocations during POP extraInfo parsing and by switching numeric getters from boxed Long to primitive long, while keeping the wire format unchanged.
Changes:
- Refactor
parseStartOffsetInfo/parseMsgOffsetInfo/parseOrderCountInfoto parse entries via separator indices instead ofsplit(...), reducing intermediate allocations. - Change
getCkQueueOffset/getPopTime/getInvisibleTimereturn types fromLongtolong. - Extend
ExtraInfoUtilTestwith round-trip, getter, and malformed-input coverage.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| remoting/src/main/java/org/apache/rocketmq/remoting/protocol/header/ExtraInfoUtil.java | Removes split-based parsing allocations and updates three numeric getters to return primitives. |
| remoting/src/test/java/org/apache/rocketmq/remoting/protocol/header/ExtraInfoUtilTest.java | Adds tests for round-trip parsing, primitive getter behavior, and malformed entry rejection. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (split.length != 3) { | ||
| long separators = locateEntrySeparators(one); | ||
| if (separators < 0) { | ||
| throw new IllegalArgumentException("parse msgOffsetMap error, " + msgOffsetMap); |
| /** | ||
| * Locates the two {@link MessageConst#KEY_SEPARATOR} positions of an entry laid out as | ||
| * {@code retryFlag queueId value}, packed as {@code (sep1 << 32) | sep2}. Returns a negative | ||
| * value when the entry does not have exactly three non-empty fields, mirroring the previous | ||
| * split-based validation without allocating the intermediate array. | ||
| */ |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #10975 +/- ##
=============================================
- Coverage 48.57% 48.55% -0.02%
- Complexity 13674 13684 +10
=============================================
Files 1381 1381
Lines 101475 101498 +23
Branches 13190 13195 +5
=============================================
- Hits 49292 49286 -6
- Misses 46180 46183 +3
- Partials 6003 6029 +26 ☔ 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.
Summary
This PR reduces allocations in ExtraInfoUtil by switching getter return types from boxed Long to primitive long (using Long.parseLong instead of Long.valueOf), and replacing String.split() with manual indexOf-based parsing in parseMsgOffsetInfo. The new locateEntrySeparators method packs two separator positions into a single long to avoid allocating intermediate String[] arrays, and buildEntryKey constructs the map key with a pre-sized StringBuilder.
The changes are correct and well-structured. The locateEntrySeparators validation logic properly handles edge cases (empty fields, too many separators). The return type change from Long to long is safe since these methods throw on invalid input and never returned null. Tests cover the round-trip behavior and malformed input scenarios.
LGTM — solid allocation reduction on a hot parsing path.
Automated review by github-manager-bot
Which Issue(s) This PR Fixes
Fixes #10973
Brief Description
Every POP response and every ACK goes through
ExtraInfoUtil. Two allocation sources removed without touching the wire format:parseStartOffsetInfo/parseMsgOffsetInfo/parseOrderCountInfousedsplitper entry (aString[]plus three substrings), then re-concatenated the first two fields into a map key;parseMsgOffsetInfosplit the offset list again withsplit(","). Entries are now walked withindexOfand the map key is built withStringBuilder.append(CharSequence, int, int), so the arrays and most substrings are gone (the value keeps one substring forparseLong; the Java 8 target has no range-parse API).getCkQueueOffset/getPopTime/getInvisibleTimereturned boxedLongalthough every caller in the repository assigns the result straight to along. They now returnlong.Notes for reviewers:
"@a"); the new validation rejects them with the sameIllegalArgumentExceptionused for other malformed shapes. Builder-produced wire strings are unaffected, covered by new round-trip tests.How Did You Test This Change?
ExtraInfoUtilTestextended with round-trip (normal + retry topic, multi-entry), getter, and malformed-input cases, 5/5 pass;AckMessageProcessorTest/ChangeInvisibleTimeProcessorTest/PopMessageProcessorTest25/25;MQClientAPIImplTest133/133.mqadmin setConsumeMode -m POP, producer 64 threads + consumer 20 threads, consume TPS steady at 150-154k, pop path confirmed active via pop.log, 3 interleaved trials per side):No regression on either side; the allocation saving itself (tens of bytes per response) is below GC-count resolution, so this is a cleanup-level optimization on a hot path plus stricter rejection of corrupt extraInfo input.