[CELEBORN-2395] Reduce allocations in Utils.split* methods - #3771
Closed
yew1eb wants to merge 1 commit into
Closed
Conversation
yew1eb
changed the title
yew1eb
added a commit
to yew1eb/celeborn
that referenced
this pull request
Jul 28, 2026
Kalvin2077
approved these changes
Jul 29, 2026
sunchao
approved these changes
Jul 30, 2026
RexXiong
approved these changes
Jul 30, 2026
SteNicholas
pushed a commit
that referenced
this pull request
Aug 3, 2026
## What changes were proposed in this pull request?
Rewrite the three `String.split("-")`-based key splitters in `Utils` to locate the `-` separator once and slice with `substring`, instead of regex-splitting and rejoining:
- `splitShuffleKey` → `lastIndexOf('-')` + two `substring` calls
- `splitPartitionLocationUniqueId` → `lastIndexOf('-')` + two `substring` calls
- `splitAttemptKey` → `indexOf('-')` + two `substring` calls
This removes the regex compilation, the intermediate `String[]`, the `dropRight(1)` array copy, and the `StringBuilder` + new `String` from `mkString`.
## Why are the changes needed?
`splitShuffleKey` is on the per-push-message hot path, called from both `recordAppActiveConnection` (default-on per-app metrics) and `checkAuth`. A production async-profiler CPU flame graph on `celeborn-worker` (80,611 samples; true stack-reconstruction, self sum = 100%) shows the frame's self cost is near zero, but its inclusive cost is ~4.23%, of which ~2.95% comes through the default-on `recordAppActiveConnection` path and ~1.27% through the always-on `checkAuth` path.
<img width="1619" height="260" alt="image" src="https://github.com/user-attachments/assets/6a807653-fcdd-4037-8d16-117ebe70046e" />
<img width="1686" height="244" alt="image" src="https://github.com/user-attachments/assets/240be695-4985-4f8a-abc1-e32c21c6d0ee" />
The allocation reduction also lowers young-GC pressure under high push QPS.
## Does this PR resolve a correctness bug?
No
## Does this PR introduce any user-facing change?
No
## How was this patch tested?
- Added unit tests in `UtilsSuite` covering `splitShuffleKey` (including `applicationId` containing `-`, plus a `makeShuffleKey` round-trip), `splitPartitionLocationUniqueId`, and `splitAttemptKey`.
- `./build/mvn -pl common -Dtest=UtilsSuite test` — all `split*` tests pass. (Two unrelated `CelebornConfSuite` failures — `Fallback to parent module's config...` and `rpc_service and rpc_client...` — are pre-existing on a clean `upstream/main`; verified by stashing this change and re-running.)
- `./build/mvn -pl common spotless:check` — clean.
Closes #3771 from yew1eb/CELEBORN-2395.
Authored-by: yew1eb <yew1eb@gmail.com>
Signed-off-by: Nicholas Jiang <programgeek@163.com>
Member
|
Thanks. Merged to main(v1.0.0) and branch-0.7(v0.7.0). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Rewrite the three
String.split("-")-based key splitters inUtilsto locate the-separator once and slice withsubstring, instead of regex-splitting and rejoining:splitShuffleKey→lastIndexOf('-')+ twosubstringcallssplitPartitionLocationUniqueId→lastIndexOf('-')+ twosubstringcallssplitAttemptKey→indexOf('-')+ twosubstringcallsThis removes the regex compilation, the intermediate
String[], thedropRight(1)array copy, and theStringBuilder+ newStringfrommkString.Why are the changes needed?
splitShuffleKeyis on the per-push-message hot path, called from bothrecordAppActiveConnection(default-on per-app metrics) andcheckAuth. A production async-profiler CPU flame graph onceleborn-worker(80,611 samples; true stack-reconstruction, self sum = 100%) shows the frame's self cost is near zero, but its inclusive cost is ~4.23%, of which ~2.95% comes through the default-onrecordAppActiveConnectionpath and ~1.27% through the always-oncheckAuthpath.The allocation reduction also lowers young-GC pressure under high push QPS.
Does this PR resolve a correctness bug?
No
Does this PR introduce any user-facing change?
No
How was this patch tested?
UtilsSuitecoveringsplitShuffleKey(includingapplicationIdcontaining-, plus amakeShuffleKeyround-trip),splitPartitionLocationUniqueId, andsplitAttemptKey../build/mvn -pl common -Dtest=UtilsSuite test— allsplit*tests pass. (Two unrelatedCelebornConfSuitefailures —Fallback to parent module's config...andrpc_service and rpc_client...— are pre-existing on a cleanupstream/main; verified by stashing this change and re-running.)./build/mvn -pl common spotless:check— clean.