yew1eb opened a new pull request, #3771:
URL: https://github.com/apache/celeborn/pull/3771
## Description
`Utils.splitShuffleKey` / `splitPartitionLocationUniqueId` /
`splitAttemptKey` used `String.split("-")` (regex + `String[]`) plus
`dropRight(1).mkString("-")` (array copy + `StringBuilder` + new `String`) to
slice the trailing segment. Since the trailing segment (`shuffleId` / `epoch` /
`attemptId`) is always the last `-`-delimited value, a single
`lastIndexOf('-')` / `indexOf('-')` plus two `substring` calls suffices — zero
regex, zero intermediate array, zero `StringBuilder`.
`splitShuffleKey` in particular sits on the per-push-message hot path
(`recordAppActiveConnection` + `checkAuth`).
### Correctness note
`applicationId` may itself contain `-` (e.g. Spark's
`application_<timestamp>_<id>`), so `lastIndexOf` (not `indexOf`) is required
for `splitShuffleKey` to slice only the trailing `shuffleId`. For
`splitPartitionLocationUniqueId` / `splitAttemptKey`, both segments are `Int`
and never contain `-`, so `lastIndexOf` / `indexOf` are equivalent and safe.
Pure refactor, no behavioral change.
## Evidence
Production async-profiler CPU flame graph on `celeborn-worker` (80,611
samples; true stack-reconstruction, self sum = 100%). The frame's self cost is
near zero — the cost is inclusive, in the JDK methods it calls. By call site
(inclusive):
| call site | inclusive |
|---|---|
| `WorkerSource.recordAppActiveConnection` → `splitShuffleKey` | 2.95% |
| `PushDataHandler.checkAuth` → `splitShuffleKey` | 1.27% |
| total | ~4.23% |
Both are on the push path. The 2.95% one is default-on per-app metrics
(disable via `celeborn.metrics.worker.appLevel.enabled=false`); the ~1.27%
checkAuth path is always-on.
## How was this patch tested?
- New unit tests in `UtilsSuite` covering `splitShuffleKey` (incl.
`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 #CELEBORN-2395.
--
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]
