Skip to content

[ISSUE #10973] Reduce allocation in ExtraInfoUtil parsing and primitive getters - #10975

Open
wang-jiahua wants to merge 1 commit into
apache:developfrom
wang-jiahua:perf/extra-info-util-parse
Open

[ISSUE #10973] Reduce allocation in ExtraInfoUtil parsing and primitive getters#10975
wang-jiahua wants to merge 1 commit into
apache:developfrom
wang-jiahua:perf/extra-info-util-parse

Conversation

@wang-jiahua

Copy link
Copy Markdown
Contributor

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:

  1. parseStartOffsetInfo / parseMsgOffsetInfo / parseOrderCountInfo used split per entry (a String[] plus three substrings), then re-concatenated the first two fields into a map key; parseMsgOffsetInfo split the offset list again with split(","). Entries are now walked with indexOf and the map key is built with StringBuilder.append(CharSequence, int, int), so the arrays and most substrings are gone (the value keeps one substring for parseLong; the Java 8 target has no range-parse API).
  2. getCkQueueOffset / getPopTime / getInvisibleTime returned boxed Long although every caller in the repository assigns the result straight to a long. They now return long.

Notes for reviewers:

  • The getter change is source-compatible for all in-repo callers (none rely on nullability or identity) but binary-incompatible for externally compiled bytecode, which needs a recompile against the new remoting artifact.
  • The previous split-based validation half-accepted corrupt entries with empty fields or trailing separators (producing keys like "@a"); the new validation rejects them with the same IllegalArgumentException used for other malformed shapes. Builder-produced wire strings are unaffected, covered by new round-trip tests.

How Did You Test This Change?

  • ExtraInfoUtilTest extended with round-trip (normal + retry topic, multi-entry), getter, and malformed-input cases, 5/5 pass; AckMessageProcessorTest / ChangeInvisibleTimeProcessorTest / PopMessageProcessorTest 25/25; MQClientAPIImplTest 133/133.
  • 4-node cluster A/B in POP mode (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):
    • broker side (remoting jar swapped on the broker): young GC per million consumed msgs 2.60/2.62/2.70 (base) vs 2.63/2.64/2.62 (patch) — parity;
    • client side (remoting jar swapped on the consumer): 1.10/1.08/1.10 vs 1.11/1.10/1.10 — parity.

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.

Copilot AI lite review requested due to automatic review settings August 27, 2026 08:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 / parseOrderCountInfo to parse entries via separator indices instead of split(...), reducing intermediate allocations.
  • Change getCkQueueOffset / getPopTime / getInvisibleTime return types from Long to long.
  • Extend ExtraInfoUtilTest with 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);
Comment on lines +304 to +309
/**
* 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-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.50000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 48.55%. Comparing base (e348efa) to head (1e273d8).

Files with missing lines Patch % Lines
...cketmq/remoting/protocol/header/ExtraInfoUtil.java 97.50% 0 Missing and 1 partial ⚠️
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.
📢 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 RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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] Reduce allocation in ExtraInfoUtil POP info parsing and return primitives from numeric getters

4 participants