Skip to content

[ISSUE #10969] Hoist per-batch constant min/max offset strings out of message loop in PullAPIWrapper - #10971

Open
wang-jiahua wants to merge 1 commit into
apache:developfrom
wang-jiahua:perf/pull-result-offset-string
Open

[ISSUE #10969] Hoist per-batch constant min/max offset strings out of message loop in PullAPIWrapper#10971
wang-jiahua wants to merge 1 commit into
apache:developfrom
wang-jiahua:perf/pull-result-offset-string

Conversation

@wang-jiahua

Copy link
Copy Markdown
Contributor

Which Issue(s) This PR Fixes

Fixes #10969

Brief Description

In PullAPIWrapper#processPullResult, the per-message loop calls Long.toString(pullResult.getMinOffset()) and Long.toString(pullResult.getMaxOffset()) for every message, although both values are constant within one pull response. A 32-message batch allocates 64 identical short-lived strings where 2 suffice, on every pull response of every push/pull consumer.

This PR hoists the two Long.toString calls out of the loop and reuses the two strings for the whole batch. No behavior change: property values are identical and putProperty semantics are untouched.

How Did You Test This Change?

  • PullAPIWrapperTest passes (11/11).
  • Joint A/B benchmark on a 4-node cluster (producer 64 threads + consumer 20 threads, 1 KiB body; only the consumer-side client jar swapped per arm; 3 interleaved 60 s trials): consumer young GC per million consumed messages 1.10/1.06/1.17 (base) vs 1.08/1.06/1.07 (patch), consume TPS flat — no regression. The saving itself is tens of short-lived strings per batch, below GC-count resolution; this is a cleanup-level optimization on a hot path.

Copilot AI lite review requested due to automatic review settings August 27, 2026 06:35

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 PullAPIWrapper#processPullResult by reducing per-message allocations when annotating pulled messages with batch-level min/max offsets, aligning with the hot-path allocation reduction described in Issue #10969.

Changes:

  • Hoists Long.toString(pullResult.getMinOffset()/getMaxOffset()) out of the per-message loop.
  • Reuses the resulting minOffset/maxOffset strings for all messages in the pull batch.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread client/src/main/java/org/apache/rocketmq/client/impl/consumer/PullAPIWrapper.java Outdated

@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.

LGTM. Clean optimization — hoisting the two Long.toString() calls out of the per-message loop is correct since pullResult.getMinOffset() and pullResult.getMaxOffset() are constant within a single pull response. Reduces unnecessary short-lived string allocations on a hot path with zero behavior change.


Automated review by github-manager-bot

@codecov-commenter

codecov-commenter commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.50%. Comparing base (e348efa) to head (634e146).

Files with missing lines Patch % Lines
.../rocketmq/client/impl/consumer/PullAPIWrapper.java 76.92% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             develop   #10971      +/-   ##
=============================================
- Coverage      48.58%   48.50%   -0.08%     
+ Complexity     13676    13653      -23     
=============================================
  Files           1381     1381              
  Lines         101475   101476       +1     
  Branches       13190    13191       +1     
=============================================
- Hits           49299    49220      -79     
- Misses         46174    46241      +67     
- Partials        6002     6015      +13     

☔ 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.

@wang-jiahua
wang-jiahua force-pushed the perf/pull-result-offset-string branch from 851b437 to 634e146 Compare August 28, 2026 01:35

@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

Hoists Long.toString(pullResult.getMinOffset()) and Long.toString(pullResult.getMaxOffset()) out of the per-message loop in PullAPIWrapper#processPullResult, since both values are constant within a single pull response. Also adds an isEmpty() guard to skip string allocation entirely when the filtered list is empty.

Clean, correct optimization. For a 32-message batch this eliminates 64 unnecessary short-lived string allocations per pull response.

LGTM.


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] Hoist per-batch constant MIN_OFFSET/MAX_OFFSET strings out of the per-message loop in PullAPIWrapper

4 participants