[ISSUE #11003] Reduce allocations in hot-path request headers' toString() - #11004
[ISSUE #11003] Reduce allocations in hot-path request headers' toString()#11004qianye1001 wants to merge 2 commits into
Conversation
![github-coding-agent[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…toString()
Replace Guava MoreObjects.toStringHelper with a pre-sized StringBuilder in the
toString() of 14 high-frequency send / consume / ack / offset request headers.
The helper form allocates a ToStringHelper plus one ValueHolder linked-list node
per add() call, resolves getClass().getSimpleName(), and grows an internal
StringBuilder(32) by repeated copying, before traversing the list to render.
For a header with N fields that is about N+3 short-lived objects per call.
The pre-sized builder keeps the rendered output byte-for-byte identical,
including omitNullValues() semantics and the conditional
.add("isLiteConsumer", isLiteConsumer ? true : null) entry in
NotificationRequestHeader.
Measured at the module's current target 1.8: 1.62x-2.60x faster toString() and
32%-48% less allocation per call.
Verified byte-identical by a differential test that loads the original and the
modified classes in separate class loaders and compares toString() over
560,483 cases (all-default / all-null / all-non-null / each field individually
null / each field individually set / boolean fields in both states / 40,000
randomized adversarial value sets per class). mvn -pl remoting test passes
(174 tests) and checkstyle reports 0 violations.
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Replaces Guava MoreObjects.toStringHelper with pre-sized StringBuilder in 14 hot-path request header toString() methods. Allocation reduced by 36–48% with 1.8–2.1x throughput improvement. Semantics are preserved correctly:
- Primitive-typed fields (
int queueId,long pollTime,long bornTime) are always emitted — matches original behavior since they can never be null. - Nullable fields (
String,Integer,Boolean) are properly null-guarded with thefirstflag pattern, preserving the originalomitNullValues()contract. - The
isLiteConsumer ? true : nullconditional inNotificationRequestHeaderis correctly materialized into a localObjectand null-checked.
Benchmarks are convincing (560K+ differential test cases, per-class allocation measurements). CI is still running but misspell-check and check-license already pass.
LGTM — clean, well-tested, and a meaningful reduction in GC pressure on the request path.
Automated review by github-manager-bot
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #11004 +/- ##
=============================================
- Coverage 48.74% 48.71% -0.04%
- Complexity 13736 13751 +15
=============================================
Files 1381 1381
Lines 101562 101669 +107
Branches 13210 13255 +45
=============================================
+ Hits 49505 49523 +18
- Misses 46007 46105 +98
+ Partials 6050 6041 -9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…en capacity estimate Scope: drop GetMaxOffsetRequestHeader, GetMinOffsetRequestHeader, RecallMessageRequestHeader and QueryConsumerOffsetRequestHeader, keeping the 10 headers on the core send / consume / ack / offset-commit data path. Those four are now byte-identical to develop. Capacity estimate: size each StringBuilder from the fixed skeleton (class name + field names + separators) plus a per-field value allowance based on realistic widths instead of a flat guess - 50 for topic/group strings, 128 for properties/subscription/extraInfo, 3 for queueId/defaultTopicQueueNums, 2 for maxMsgNums, 20 for long, 11 for other int, 5 for boolean, 16 for other strings. SendMessageRequestHeaderV2 uses compressed field names (a..n), so the allowance is applied by semantic field rather than by identifier. Measured allocation is very sensitive to this: for a 348-char rendering, sizing the builder at 16 costs 229% of the exact-fit allocation and 0.56x throughput, at the exact length it is the optimum, and at 4800 it costs 635% and 0.29x. Over-sizing is therefore not free, which is why the estimate is per-field. Re-verified against a pristine build of the merge base: 400,380 differential cases over the 10 classes, 0 mismatches. mvn -pl remoting test passes (174 tests) and checkstyle reports 0 violations.
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Replaces Guava MoreObjects.toStringHelper() with manual StringBuilder-based toString() across 10 hot-path request header classes. Clean mechanical refactor that reduces object allocation.
Review Notes
- Pre-allocated
StringBuildersizes (e.g., 391, 191) are reasonable and avoid resizing - Null-field omission logic correctly mirrors Guava's
omitNullValues()behavior - Output format (
ClassName{field=value, ...}) is compatible with the original - Primitive fields in
NotificationRequestHeader(queueId, pollTime, bornTime) could skip the!firstguard since they're never null, but this is cosmetic and doesn't affect correctness
LGTM — straightforward performance improvement with no behavioral changes.
Automated review by github-manager-bot
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Replaces Guava MoreObjects.toStringHelper with pre-sized StringBuilder in 10 hot-path request headers. Exceptional PR quality.
Key strengths:
- 40-60% allocation reduction per toString() call on send/consume/ack/offset-commit paths
- Per-class capacity estimates based on realistic field widths, not flat guesses
- Semantics preserved exactly: omitNullValues() becomes conditional appends, primitive fields always emitted
- 400,380 differential test cases (including 40,000 adversarial random sets per class) with 0 mismatches
- Negative control test verifies the test catches regressions (the isLiteConsumer case)
- Clear scope: 4 deliberately-excluded headers still show their MoreObjects references
Minor observation:
The pre-sized StringBuilder approach is optimal for JDK 8 target. When the project eventually moves to JDK 9+ bytecode, plain concatenation would be even better (invokedynamic/StringConcatFactory). The PR body notes this as a worthwhile follow-up.
LGTM — exemplary PR with thorough testing and clear documentation 👍
Automated review by github-manager-bot
3 participants
Which Issue(s) This PR Fixes
Brief Description
Replaces Guava
MoreObjects.toStringHelperwith a pre-sizedStringBuilderin thetoString()of the 10 request headers on the core send / consume / ack / offset-commit data path, underremoting/.../protocol/header/.The helper form allocates, per call and before the resulting
Stringis even built:ToStringHelperobject;ValueHolderhead plus oneValueHoldernode per.add(...)(a singly linked list);getClass().getSimpleName(), which itself allocates (getName()+substring);StringBuilder(32)that grows through repeatedArrays.copyOfonce the rendering exceeds 32 characters;and then traverses that list to render. For a header with N fields that is roughly N + 3 short-lived objects plus an O(N) traversal per call. These
toString()methods are on hot paths (request logging, exception messages, troubleshooting output), so at high throughput this is a steady stream of short-lived garbage and CPU spent purely on rendering.The replacement uses a single chained
appendsequence with a per-class initial capacity.Semantics preserved exactly:
.omitNullValues()(AckMessageRequestHeader,ChangeInvisibleTimeRequestHeader,NotificationRequestHeader) becomes conditional appends that skip null entries. Primitive-typed fields are always emitted, matching today's behaviour since they autobox to a non-null value.NotificationRequestHeader's.add("isLiteConsumer", isLiteConsumer ? true : null)is preserved by evaluating the expression into a local and null-checking it, so the entry is still rendered only whentrue.com.google.common.base.MoreObjectsimport is dropped from each touched file.Capacity estimate
The initial capacity is derived per class from the fixed skeleton (class name + field names +
=/,separators) plus a per-field value allowance based on realistic widths rather than a flat guess:properties/subscription/extraInfoqueueId,defaultTopicQueueNumsmaxMsgNums/maxMsgNumlong/Long(timestamps, offsets)int/Integerboolean/BooleanSendMessageRequestHeaderV2uses compressed field names (a..n), so the allowance is applied by semantic field (a=producerGroup,b=topic,c=defaultTopic,i=properties, ...) rather than by identifier.This matters more than it looks. Measured on a 348-character rendering, allocation and throughput by initial capacity:
javacemits for+)new char[cap]is allocated and zero-filled in proportion to the capacity, whiletoString()copies only the real length - so slack is pure waste and it accelerates eden filling. Over-sizing is therefore not free, which is why the estimate is per-field.Measured effect
On the real classes, JDK 8 runtime, this module's
target 1.8bytecode, realistic field values (~50-char topic/group, ~120-charproperties). Allocation measured withThreadMXBean.getThreadAllocatedBytes:SendMessageRequestHeader(send)SendMessageRequestHeaderV2(sendV2)PopMessageRequestHeader(pop)PopLiteMessageRequestHeader(popLite)PullMessageRequestHeader(pull)NotificationRequestHeader(notification, omitNullValues)AckMessageRequestHeader(ack, omitNullValues)ChangeInvisibleTimeRequestHeader(omitNullValues)UpdateConsumerOffsetRequestHeader(offset commit)ConsumerSendMsgBackRequestHeader(retry)Note on the choice of form: plain
+concatenation is more concise and on a JDK 9+ bytecode target compiles toinvokedynamic/StringConcatFactory.makeConcatWithConstants, which beats anything hand-written - measured 4.53x and -73% allocation on JDK 21. But at this module'starget 1.8,javaclowers+tonew StringBuilder()with the default capacity of 16, so the growth-and-copy steps cancel out most of the benefit of dropping Guava: measured on a 348-char rendering,+gives 1.27x and -2.6% allocation versus 2.07x and -48.3% for the pre-sized builder. The pre-sized builder also stays good on newer targets (1.95x on JDK 21), so it is not a liability; if the project later raises the bytecode target, switching these to plain concatenation would be a worthwhile follow-up.Scope note:
GetMaxOffsetRequestHeader,GetMinOffsetRequestHeader,QueryConsumerOffsetRequestHeaderandRecallMessageRequestHeaderwere deliberately dropped from this change and are byte-identical todevelop. Together with the admin/low-frequency headers, the remainingtoStringHelperusers can be a follow-up.How Did You Test This Change?
The rendered strings end up in logs that people and external tooling parse, so the requirement is that output stays byte-for-byte identical. Testing focused on proving that rather than on eyeballing the diff.
1. Differential test against the original compiled bytecode (primary evidence). A pristine worktree at the merge base (
bee586bcd) and the modified tree are compiled into two separate output directories, then loaded into two independent class loaders in one JVM. For each header the test instantiates both versions, applies an identical field-value plan to both via reflection, and comparestoString(). This compares against the real original implementation, so it does not depend on any assumption about how Guava formats things.Value plans per class: all-default (exercising field initializers such as
order = Boolean.FALSE,suspend = false,unitMode = false), all-null, all-non-null, each field individually null with all others set, each field individually set with all others null, boolean/Boolean fields in both states (this is what covers the conditionalisLiteConsumerentry), and 40,000 randomized adversarial value sets per class drawing from null, empty string,\r,\n,\t,\r\n\t, other control characters (\u0001,\u0002), non-ASCII and surrogate-pair text, 300- and 2000-character strings,%RETRY%-prefixed groups, hex message ids, andInteger/LongMAX_VALUE/MIN_VALUE.Result: 400,380 cases, 0 mismatches across the 10 classes.
2. Negative control on the test itself. An earlier iteration of this change silently dropped the conditional
isLiteConsumerentry (its.add(...)value is a ternary rather than a plain field reference, so a naive rewrite lost it). Deleting that handling from the final code makes the differential test fail immediately and print the exact divergence:This confirms the test detects this class of regression instead of passing vacuously.
3. Bytecode-level scope check. Decompiling the built classes confirms
toString()in the 10 touched headers has zeroMoreObjectsreferences, while the 4 deliberately-excluded headers still show theirs (5, 4, 6 and 5 references respectively) - i.e. nothing outside the intended scope was altered. For an earlier revision of this change the same technique confirmed that all non-toStringmethods were byte-identical to the original.4. Existing test suite and style gates.
mvn -pl remoting test: 174 tests, 0 failures, 0 errors (includesSendMessageRequestHeaderV2Test,RpcRequestHeaderTest,ProxyProtocolTest,FastCodesHeaderTest).style/rmq_checkstyle.xmlat thevalidatephase: 0 violations (this also confirms dropping the now-unusedMoreObjectsimport is clean).mvn -pl remoting -am compile: BUILD SUCCESS.5. Diff scope.
git diff --stat origin/developtouches exactly the 10 intended header files and nothing else; each file's change is confined to itstoString()body plus removal of the unused import.