Support device buffer for the new libcurl multi poll-based backend (5/n) - #966
Conversation
Co-authored-by: Lawrence Mitchell <wence@gmx.li>
|
Sending this PR to draft. The reactor thread logic is very messy and can potentially be improved by RAII. Refactoring to have a cleaner code now. |
Performance results
The results above show that: (1) For EC2-S3, there exists certain level of network bandwidth variance. (2) Comparing the best results, the new multi-poll backend is ~3% slower than the existing easy thread pool backend, with the benefit of thread usage cut by a factor of 16. |
Event pool vs on-the-fly creation/destructionOn a temporary branch, the event pool was modified such that the
cc @madsbk |
| /** | ||
| * @brief Construct a barrier carrying `cuda_context` as metadata. | ||
| * | ||
| * @param cuda_context The CUDA context that pred's H2Ds will land in. Stored only for callers |
There was a problem hiding this comment.
| * @param cuda_context The CUDA context that pred's H2Ds will land in. Stored only for callers | |
| * @param cuda_context The CUDA context that pread's H2Ds will land in. Stored only for callers |
| explicit RemoteMultiAggregateContext(std::size_t num_subranges); | ||
|
|
||
| /** | ||
| * @brief Per-pread event watermark for the device-buffer path. |
There was a problem hiding this comment.
Is "watermark" a common term of art here? I'm not familiar with what this means, it might be made up by an LLM?
There was a problem hiding this comment.
No jargon used here. The per-pread, per-thread event from the IoEventBarrier is simply used repeatedly this way:
H2D (from pread 1) -> record event 1 ->
H2D (from pread 1) -> record the same event (from pread 1) ->
H2D (from pread 2), record event 2"
...
Anyway I've removed the mention of "watermark" to avoid confusion.
| { | ||
| } | ||
|
|
||
| ConcurrentRequestLimiter::Slot& ConcurrentRequestLimiter::Slot::operator=(Slot&& o) noexcept |
There was a problem hiding this comment.
Let's use other instead of o. One character variables are generally not favored in our codebases.
There was a problem hiding this comment.
Done. Also replaced other uses of o with other.
| detach_from_multi(_multi, _easy); | ||
| } | ||
|
|
||
| CurlMultiAttachment::CurlMultiAttachment(CurlMultiAttachment&& o) noexcept |
There was a problem hiding this comment.
Let's use other here and throughout instead of o.
| { | ||
| } | ||
|
|
||
| CurlMultiAttachment& CurlMultiAttachment::operator=(CurlMultiAttachment&& o) noexcept |
There was a problem hiding this comment.
Do we need to explicitly = delete; the copy constructor and copy assignment? (Trying to remember the rules.)
There was a problem hiding this comment.
The move functions cause the copy functions to be deleted, but the rule of five recommends explicit deletion for readability.
| } | ||
| } | ||
|
|
||
| // Admission walk over the reactor-private _pending. Each entry is either admitted to libcurl |
There was a problem hiding this comment.
"Admission walk over the reactor-private _pending." What does this mean? See if you can rewrite this.
There was a problem hiding this comment.
Simplified a bit.
vuule
left a comment
There was a problem hiding this comment.
looks good, just one potential concern
| return std::async(std::launch::deferred, | ||
| [fut = std::move(fut), io_event_barrier]() mutable -> std::size_t { | ||
| auto const n = fut.get(); | ||
| io_event_barrier->sync_all_events(); |
There was a problem hiding this comment.
sync_all_events() is documented as context-agnostic, but it synchronizes events created in the pread’s CUDA context. Make IoEventBarrier::sync_all_events() push _cuda_context before synchronizing its events, and add coverage where the future is consumed on a thread with no current CUDA context.
There was a problem hiding this comment.
So this lambda with deferred policy is evaluated at the future.get() time, where the CUDA context is allowed to be different from pread's. I've updated the doc to clarify what context-agnostic means.
I've added a unit test for sync_all_events() which is called when (1) there is no context current on the calling thread, (2) there is a different context.
|
/merge |
This PR supports device buffer for the multi poll-based remote I/O backend. In addition, this PR makes 5 improvements:
ConcurrentRequestLimiterwith RAII Slot.try_acquire()now returns a move-onlySlotthat auto-releases on destruction. Acquire/release pairing is now structurally enforced.CurlMultiAttachmentRAII guard, that removes the easy handle from the multi handle on destruction.curl_multi_wakeupcalls from the initial "per sub-range" to the current "per reactor, per pread"._inbox(shared by the submission thread and reactor thread) to a reactor-private_pendingdeque, to decouple the submission latency from reactor's admission walk.