[ISSUE #10976] Reduce per-message allocation on the proxy gRPC path - #10977
[ISSUE #10976] Reduce per-message allocation on the proxy gRPC path#10977wang-jiahua wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Reduces per-message allocations on the proxy gRPC hot path by reusing MD5 MessageDigest instances per thread and validating UTF-8 byte lengths without allocating intermediate byte arrays.
Changes:
- Reworked
BinaryUtil.calculateMd5to use a per-threadMessageDigest(ThreadLocal) and added tests for stability/correctness. - Replaced multiple
String.getBytes(UTF_8).lengthvalidations with a zero-allocationutf8Length(String)helper. - Added unit tests covering UTF-8 length calculations across ASCII, BMP, supplementary characters, and malformed surrogate cases.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| proxy/src/main/java/org/apache/rocketmq/proxy/grpc/v2/producer/SendMessageActivity.java | Introduces utf8Length and switches size checks to avoid byte[] allocations. |
| proxy/src/test/java/org/apache/rocketmq/proxy/grpc/v2/producer/SendMessageActivityTest.java | Adds direct unit test coverage for utf8Length equality vs getBytes(UTF_8).length. |
| common/src/main/java/org/apache/rocketmq/common/utils/BinaryUtil.java | Uses a per-thread cached MD5 digest to avoid repeated provider lookup/allocation. |
| common/src/test/java/org/apache/rocketmq/common/utils/BinaryUtilTest.java | Adds tests to verify MD5 correctness and that per-thread reuse resets between calls. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
This PR eliminates two per-message allocation sources on the proxy gRPC path: (1) MessageDigest.getInstance("MD5") provider lookup on every calculateMd5 call, replaced with a ThreadLocal<MessageDigest> that is reset() before each use; (2) str.getBytes(UTF_8).length for measuring UTF-8 encoded size, replaced with a branch-based utf8Length() that computes the byte count without allocating a temporary array.
Both changes are correct and well-motivated. The ThreadLocal pattern for MessageDigest is standard practice for non-thread-safe reusable objects, and reset() is properly called before each use. The utf8Length() implementation correctly handles all cases: ASCII, BMP, supplementary characters (surrogate pairs), and unpaired surrogates (matching Java's getBytes(UTF_8) replacement behavior). Tests cover all edge cases including interleaved payloads to verify the ThreadLocal does not corrupt results.
LGTM — clean, focused performance optimization with solid test coverage.
Automated review by github-manager-bot
0402c42 to
a7df129
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #10977 +/- ##
=============================================
- Coverage 48.58% 48.52% -0.06%
+ Complexity 13676 13665 -11
=============================================
Files 1381 1381
Lines 101475 101492 +17
Branches 13190 13196 +6
=============================================
- Hits 49299 49248 -51
- Misses 46174 46222 +48
- Partials 6002 6022 +20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Reduces per-message allocation on the proxy gRPC path by (1) caching MessageDigest in a ThreadLocal in BinaryUtil.calculateMd5 and (2) replacing String.getBytes(UTF_8).length with a zero-allocation utf8Length() in SendMessageActivity.
Changes look correct:
ThreadLocal<MessageDigest>withreset()before each use is the right pattern for non-thread-safe digest instances- Test interleaves calls to verify reset stability — good coverage
utf8Length()avoids byte array allocation for size validation on the hot path
Minor note: the .gitignore additions (.qoder/, profiling-tools/, etc.) appear to be local dev artifacts — consider whether these belong in a project-wide .gitignore.
LGTM.
Automated review by github-manager-bot
Which Issue(s) This PR Fixes
Fixes #10976
Brief Description
Two per-message allocation sources removed on the proxy gRPC path:
BinaryUtil.calculateMd5performed aMessageDigest.getInstance("MD5")provider lookup and created a fresh digest on every call;GrpcConverter#buildSystemPropertieshits this for every message delivered to a gRPC consumer. The digest is now kept per thread in aThreadLocal(MessageDigestis not thread safe) andreset()before each use.SendMessageActivity#buildMessageProperty(andvalidateMessageGroup) calledgetBytes(StandardCharsets.UTF_8)on every user-property key/value, the tag, each message key, and the message group, only to read.lengthfor size validation. A newutf8Length(String)helper computes the encoded length without materializing the array, matchingString.getBytes(UTF_8).lengthexactly (including the single-byte replacement for unpaired surrogates); the five call sites now use it.How Did You Test This Change?
BinaryUtilTest(new): result matches a freshMessageDigest, and stays stable across interleaved calls on the reused per-thread instance.SendMessageActivityTest#testUtf8Length: sample-by-sample equality withgetBytes(UTF_8).lengthcovering ASCII, CJK, supplementary (emoji), and unpaired surrogates; full class 12/12.rocketmq-client-java5.0.7 (producer 8 threads with multi-byte user properties exercisingutf8Length; SimpleConsumer 4 threads exercising the digest path), swapping the proxy'srocketmq-common/rocketmq-proxyjars per arm, 3 interleaved trials plus 1 reversed-order control: ~9.4k send TPS / ~5.1k consume TPS with zero failures in all 8 arms; proxy young GC counts showed a pure positional artifact (first arm of each pair always 9, second always 10, independent of the jar — confirmed by the reversed-order control), i.e. parity after correction. No regression; the allocation saving is below GC-count resolution, so this is a cleanup-level optimization on the proxy hot path.