apache / apache/rocketmq

[Enhancement] Reduce allocations in hot-path request headers' toString() by replacing Guava MoreObjects.toStringHelper

Open
#11,003 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
Java
Stars
22.6k
Forks
12k
Avg merge
3d 1h
Merged PRs (30d)
27

Description

### Before Creating the Enhancement Request

- [X] I have confirmed that this should be classified as an enhancement rather than a bug/feature.

### Summary

Replace Guava `MoreObjects.toStringHelper` with a pre-sized `StringBuilder` in the `toString()` of the high-frequency request header classes under `remoting/src/main/java/org/apache/rocketmq/remoting/protocol/header/`. The rendered output stays byte-for-byte identical; the change removes roughly N+3 short-lived allocations and an O(N) linked-list traversal per call for a header with N fields.

Scope (14 send / consume / ack / offset hot-path headers): `SendMessageRequestHeader`, `SendMessageRequestHeaderV2`, `PopMessageRequestHeader`, `PopLiteMessageRequestHeader`, `PullMessageRequestHeader`, `NotificationRequestHeader`, `AckMessageRequestHeader`, `ChangeInvisibleTimeRequestHeader`, `QueryConsumerOffsetRequestHeader`, `UpdateConsumerOffsetRequestHeader`, `GetMaxOffsetRequestHeader`, `GetMinOffsetRequestHeader`, `ConsumerSendMsgBackRequestHeader`, `RecallMessageRequestHeader`.

Measured on the module's current build target (JDK 8 / `target 1.8`): **1.62x - 2.60x faster `toString()` and 32% - 48% less allocation per call**.

### Motivation

These headers render themselves with `MoreObjects.toStringHelper`, e.g. `SendMessageRequestHeader`:

```java
return MoreObjects.toStringHelper(this)
.add("producerGroup", producerGroup)
.add("topic", topic)
... // 13 fields in total
.toString();
```

Per invocation this allocates, before the resulting `String` is even built:

1. one `ToStringHelper` object;
2. one `ValueHolder` head plus **one `ValueHolder` node per `.add(...)`**, forming a singly linked list;
3. `getClass().getSimpleName()`, which itself allocates (`getName()` + `substring`);
4. an internal `StringBuilder(32)` that grows through repeated `Arrays.copyOf` whenever the rendering exceeds 32 characters;

and then traverses that linked list to render. For a header with N fields this is about **N + 3 short-lived objects plus an O(N) traversal per call**.

`toString()` on these headers sits on hot paths: it is invoked whenever a header is rendered for logging or diagnostics (request logging, exception messages, troubleshooting output). At high message throughput this becomes a continuous stream of short-lived garbage and a measurable amount of CPU spent purely on rendering, which also shows up as extra young-generation collection work.

### Describe the Solution You'd Like

Use a **pre-sized `StringBuilder`** with direct `append` calls:

- **Plain headers** - a single chained `append` sequence. The initial capacity is derived from the 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 here: a default-capacity builder needs 5-6 growth-and-copy steps to reach a ~350 character rendering.
- **Headers using `.omitNullValues()`** (`AckMessageRequestHeader`, `ChangeInvisibleTimeRequestHeader`, `NotificationRequestHeader`) - conditional appends that skip null values, preserving the exact omission semantics. Primitive-typed fields are always emitted, which matches today's behaviour since they autobox to a non-null value.
- **Conditional entries** such as `.add("isLiteConsumer", isLiteConsumer ? true : null)` in `NotificationRequestHeader` are preserved by evaluating the expression into a local and null-checking it, so the entry is still rendered only when `true`.

The rendered strings must remain **byte-for-byte identical**, because they end up in logs that people and external tooling parse.

### Describe Alternatives You've Considered

**1. Plain `+` string concatenation.** More concise, and on a JDK 9+ bytecode target it compiles to `invokedynamic` / `StringConcatFactory.makeConcatWithConstants`, which beats anything hand-written - measured **4.53x** and **-73%** allocation on JDK 21.

However this module builds with `maven.compiler.source/target = 1.8`, where `javac` lowers `+` to `new StringBuilder()` with the default capacity of 16. For headers whose rendering runs to a few hundred characters (those carrying `properties` / `subscription` / `extraInfo`) that means 5-6 growth-and-copy steps, which cancels out most of the benefit of dropping Guava. Measured for `SendMessageRequestHeader` at `target 1.8`:

| form | alloc/call | throughput vs Guava |
|---|---|---|
| `+` concatenation | 3664 B (-2.6%) | 1.27x |
| pre-sized `StringBuilder` | 1944 B (-48.3%) | **2.07x** |

So plain concatenation was rejected for now: it is the worse option under the project's current build target. The pre-sized builder also stays good on newer targets (1.95x on JDK 21), so it does not become a liability. **If the project later raises the bytecode target, switching these to plain `+` concatenation would be a worthwhile follow-up.**

**2. Removing or gating the `toString()` calls at the call sites.** Out of scope: it changes observable logging behaviour rather than making the existing rendering cheaper.

**3. Also converting the remaining ~10 header classes that use `toStringHelper`.** Left out deliberately - those are admin / low-frequency headers, and keeping them out makes this change easier to review. They can follow up separately.

### Additional Context

#### Measured effect

Microbenchmark against the real classes, JDK 8 runtime, the module's `target 1.8` bytecode, realistic field values (including a ~120 character `properties`). Allocation measured with `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** |

#### Correctness verification

A differential test loads the original and the modified compiled classes in two separate class loaders, applies identical field values to both instances and compares `toString()`. Value plans cover: all-default, all-null, all-non-null, **each field individually null**, **each field individually set**, boolean fields in both states (this is what covers the conditional `isLiteConsumer` entry), plus 40,000 randomized adversarial value sets per class (null, empty, `\r\n\t`, other control characters, 300- and 2000-character strings, non-ASCII, `Integer`/`Long` `MAX_VALUE`/`MIN_VALUE`).

- Differential test: **560,483 cases, 0 mismatches**
- `mvn -pl remoting test`: **174 tests, 0 failures**
- checkstyle (`style/rmq_checkstyle.xml`, `validate` phase): **0 violations**

As a check on the test itself, deleting the conditional `isLiteConsumer` handling makes the differential test fail immediately and prints the exact divergence, confirming it can detect this class of regression rather than passing vacuously.

Verified against `develop` at `bee586bcd`.

Contributor guide

Open the contributing guide

Research direction

Start with the 14 named request-header classes under remoting/src/main/java/org/apache/rocketmq/remoting/protocol/header/, using SendMessageRequestHeader as the initial entry point. Review the existing toStringHelper output and run the differential verification described in the issue, followed by mvn -pl remoting test and the validate phase. Done means byte-for-byte identical output, including null omission and conditional fields, with the stated remoting tests and checkstyle passing.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
api, backend, performance
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.