You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "ForVic (via GitHub)" <gi...@apache.org> on 2026/10/22 08:52:35 UTC
[PR] [CELEBORN-XXXX] Swap DataPusher buffers instead of copying pushed data [celeborn]
ForVic opened a new pull request, #3892:
URL: https://github.com/apache/celeborn/pull/3892
### What changes were proposed in this pull request?
`DataPusher.addTask` copies the caller's full push buffer into an idle `PushTask` buffer before queueing it. The Spark writers only call it with a buffer they are about to reuse, so they can give the buffer away instead of copying it.
- **client**: `DataPusher.addTaskAndSwapBuffer` puts the caller's buffer into the idle `PushTask` and returns the task's previous buffer for the caller to continue with. `addTask` keeps copying and is still used by the Tez writer.
- **spark**: `HashBasedShuffleWriter` and `SortBasedPusher` use `addTaskAndSwapBuffer`. Both only flush a full buffer of `celeborn.client.push.buffer.max.size` bytes, the size of the `PushTask` buffers, so the buffer the writer gets back is the same size.
Every buffer is owned by either the writer or a single `PushTask`, so the push thread never reads a buffer the writer is still filling. The task is handed over through the same queues as before, which also publish the swapped buffer to the push thread.
### Why are the changes needed?
Spark shuffle writes copy every pushed byte once more on the task thread. That copy goes away.
Mini benchmark on one m6id.8xlarge (32 vCPU, Celeborn master and worker on the local NVMe disk, Spark 3.5.8 `local[16]`, AQE off). Each `spark-submit` caches 100M rows (`id`, two `uuid()`s, `rand()`, a short string), then repartitions them into 400 partitions three times. Main and this PR swap only the Spark client jar and alternate order every round. Medians of the warm iterations, 10 per side (5 submits x 2 iterations), p from a permutation test:
| Writer, codec | Shuffle per iteration | Map task CPU, main -> PR | Map stage wall, main -> PR |
|---|---|---|---|
| hash, lz4 | 8.8 GiB | 107.6 s -> 106.6 s (-1.0%, p=0.04) | 10.08 s -> 9.45 s (-6.2%, p=0.009) |
| sort, lz4 | 8.8 GiB | 125.0 s -> 124.1 s (-0.7%, p=0.12) | 10.90 s -> 10.82 s (-0.7%, p=0.55) |
| hash, none | 13.0 GiB | 96.2 s -> 94.6 s (-1.6%, p<0.001) | 24.65 s -> 24.59 s (-0.3%, p=0.73) |
The CPU saved, about 1 to 1.5 CPU seconds per iteration, matches the 9 to 13 GiB of memcpy removed. With compression off the CPU ranges do not overlap at all. Wall time only moves for the hash writer with lz4. The run without compression is bound by the worker, not the writer. The hash/lz4 wall gain is larger than the CPU saving alone explains, so treat it as observed on this box rather than expected in general.
Writer-side cost of handing one full buffer to `DataPusher` with a no-op push, one writer thread, median of the warm rounds:
| Buffer size | `addTask` (copy) | `addTaskAndSwapBuffer` |
|---|---|---|
| 64 KiB (default) | 4.2 us | 1.6 us |
| 256 KiB | 23.7 us | 1.9 us |
### Does this PR resolve a correctness bug?
- [ ] Yes
### Does this PR introduce _any_ user-facing change?
- [ ] Yes
### How was this patch tested?
- New `DataPushQueueSuiteJ#testAddTaskAndSwapBuffer`: pushes 1000 buffers, each filled with its own index, and checks that every returned buffer is a different array of the same size. It also checks that the push thread sees each payload intact and in order while the writer keeps filling the returned buffers.
- Existing `DataPushQueueSuiteJ`.
- The benchmark above. Both variants wrote the same amount of shuffle data in every iteration.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-XXXX] Swap DataPusher buffers instead of copying pushed data [celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #3892:
URL: https://github.com/apache/celeborn/pull/3892#issuecomment-6352340681
## [Codecov](https://app.codecov.io/gh/apache/celeborn/pull/3892?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 61.30%. Comparing base ([`04ec937`](https://app.codecov.io/gh/apache/celeborn/commit/04ec937ff0b86b952d17b90a14566a4f07caf6b9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`28e6339`](https://app.codecov.io/gh/apache/celeborn/commit/28e6339c525e3f43e776d1e8f7372e8b2d444ff1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 9 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #3892 +/- ##
============================================
+ Coverage 61.09% 61.30% +0.21%
- Complexity 346 379 +33
============================================
Files 404 404
Lines 28670 28689 +19
Branches 2828 2830 +2
============================================
+ Hits 17513 17584 +71
+ Misses 9823 9765 -58
- Partials 1334 1340 +6
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/celeborn/pull/3892?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2502] Swap DataPusher buffers instead of copying pushed data [celeborn]
Posted by "ForVic (via GitHub)" <gi...@apache.org>.
ForVic commented on PR #3892:
URL: https://github.com/apache/celeborn/pull/3892#issuecomment-6358087450
cc @SteNicholas or @RexXiong when you get the chance
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2502] Swap DataPusher buffers instead of copying pushed data [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on PR #3892:
URL: https://github.com/apache/celeborn/pull/3892#issuecomment-6409626450
@ForVic, thanks for contribution. Merged to main(v1.0.0) and branch-0.7(v0.7.1).
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2502] Swap DataPusher buffers instead of copying pushed data [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on PR #3892:
URL: https://github.com/apache/celeborn/pull/3892#issuecomment-6409590775
**Merge Summary:**
- merged into main https://github.com/apache/celeborn/commit/16642f45
*Posted by `merge_pr.py`*
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2502] Swap DataPusher buffers instead of copying pushed data [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas closed pull request #3892: [CELEBORN-2502] Swap DataPusher buffers instead of copying pushed data
URL: https://github.com/apache/celeborn/pull/3892
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org