qianye1001 opened a new pull request, #11004:
URL: https://github.com/apache/rocketmq/pull/11004
<!-- Please make sure the target branch is right. In most case, the target
branch should be `develop`. -->
### Which Issue(s) This PR Fixes
- Fixes #11003
### Brief Description
Replaces Guava `MoreObjects.toStringHelper` with a pre-sized `StringBuilder`
in the `toString()` of 14 high-frequency send / consume / ack / offset request
headers under `remoting/.../protocol/header/`.
The helper form allocates, per call and before the resulting `String` is
even built:
1. one `ToStringHelper` object;
2. one `ValueHolder` head plus **one `ValueHolder` node per `.add(...)`** (a
singly linked list);
3. `getClass().getSimpleName()`, which itself allocates (`getName()` +
`substring`);
4. an internal `StringBuilder(32)` that grows through repeated
`Arrays.copyOf` once 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 `append` sequence with an initial
capacity derived from each class's fixed skeleton (class name + field names +
separators) plus an allowance for variable-length values such as `properties`,
`subscription` and `extraInfo`. Pre-sizing matters: a default-capacity builder
needs 5-6 growth-and-copy steps to reach a ~350 character rendering.
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.
- **Conditional entries** - `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 when `true`.
- The unused `com.google.common.base.MoreObjects` import is dropped from
each touched file.
Measured at this module's current build target (JDK 8 / `target 1.8`), on
the real classes with realistic field values (including a ~120 character
`properties`), allocation measured via `ThreadMXBean.getThreadAllocatedBytes`:
| Header | alloc before | alloc after | reduction | throughput |
|---|---|---|---|---|
| `SendMessageRequestHeader` (13 fields) | 3760 B | 1944 B | **-48.3%** |
**2.07x** |
| `PullMessageRequestHeader` (15 fields) | 4072 B | 2328 B | -42.8% | 1.79x |
| `PopMessageRequestHeader` (12 fields) | 2456 B | 1432 B | -41.7% | 2.08x |
| `AckMessageRequestHeader` (6 fields, omitNullValues) | 1848 B | 1176 B |
-36.4% | 1.80x |
| `ChangeInvisibleTimeRequestHeader` (8 fields, omitNullValues) | 2088 B |
1416 B | -32.2% | 1.62x |
| `QueryConsumerOffsetRequestHeader` (4 fields) | 1104 B | 608 B | -44.9% |
**2.60x** |
Note on the choice of form: plain `+` concatenation is more concise and on a
JDK 9+ bytecode target compiles to `invokedynamic` / `makeConcatWithConstants`,
which is faster than anything hand-written (measured 4.53x and -73% allocation
on JDK 21). But at this module's `target 1.8`, `javac` lowers `+` to `new
StringBuilder()` with the default capacity of 16, so the growth-and-copy steps
cancel out most of the benefit - measured only 1.27x and -2.6% allocation for
`SendMessageRequestHeader`, 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.
The remaining ~10 header classes that also use `toStringHelper` are admin /
low-frequency ones and are intentionally left out to keep this change
reviewable.
### 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).** The original `remoting` classes (at the merge base, unmodified)
and the modified classes 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 compares `toString()`. 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`, `committed = true`, `suspend = 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 conditional `isLiteConsumer` entry), 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, and `Integer`/`Long`
`MAX_VALUE`/`MIN_VALUE`.
Result: **560,483 cases, 0 mismatches** across all 14 classes.
**2. Negative control on the test itself.** An earlier iteration of this
change silently dropped the conditional `isLiteConsumer` entry (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:
```
orig=NotificationRequestHeader{consumerGroup=V, topic=V, queueId=7,
pollTime=77, bornTime=77, order=true, attemptId=V, isLiteConsumer=true,
clientId=V}
new =NotificationRequestHeader{consumerGroup=V, topic=V, queueId=7,
pollTime=77, bornTime=77, order=true, attemptId=V, clientId=V}
```
This confirms the test detects this class of regression instead of passing
vacuously.
**3. Existing test suite and style gates.**
- `mvn -pl remoting test`: **174 tests, 0 failures, 0 errors** (includes
`SendMessageRequestHeaderV2Test`, `RpcRequestHeaderTest`, `ProxyProtocolTest`,
`FastCodesHeaderTest`).
- checkstyle with the project's `style/rmq_checkstyle.xml` at the `validate`
phase: **0 violations** (this also confirms dropping the now-unused
`MoreObjects` import is clean).
- `mvn -pl remoting -am compile`: BUILD SUCCESS.
**4. Scope check.** `git diff --stat` touches exactly the 14 intended header
files and nothing else; each file's change is confined to its `toString()` body
plus removal of the unused import.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
