Implement CUDA event pool to minimize runtime resource allocation overhead (libcurl multi poll-based backend 2/n)#919
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
| if (event == nullptr) { | ||
| // Create an event outside the lock to improve performance. | ||
| // The pool is not updated here; the returned Event object will automatically return the event | ||
| // to the pool when it goes out of scope | ||
| CUDA_DRIVER_TRY(cudaAPI::instance().EventCreate(&event, CU_EVENT_DISABLE_TIMING)); | ||
| } |
There was a problem hiding this comment.
question: Should we have a max size on this event pool? Since we never destroy events, could the pool unboundedly grow in a long-running application? I suppose it will depend on the maximum concurrency of threads issuing reads?
There was a problem hiding this comment.
I think it is probably fine to let it grow unbounded. The idea I'm trying to implement in #921 is that each pread() builds a "pread context" that gets a num_threads number of events from the pool, i.e. a single event for each thread. Each 4-MiB chunked read() originating from a specific pread() performs the following in sequence:
- I/O (file->bounce buffer ring ([WIP] Optimize easy-handle remote I/O using bounce buffer ring #916))
- H2D copy async (bounce buffer ring->device) on a per-thread, per-context stream
- Enqueue the per-
pread, per-thread, per-context event on the stream
This event is reused for all the chunks on the same thread originating from the same pread() call. So the space complexity overall is O(num_threads * num_concurrent_pread), which I think is not likely to blow up the RAM in a long running application.
But I do think that in the future a limitation on the resource pools (currently we have bounce buffer pool, this event pool, and libcurl easy handle pool) is a good feature.
| _pools[context].push_back(event); | ||
| } catch (...) { | ||
| // If returning to pool fails, destroy the event | ||
| cudaAPI::instance().EventDestroy(event); |
There was a problem hiding this comment.
question: Not checking the error code because this is called from ~Event()?
I think we should at least log an error in that case.
There was a problem hiding this comment.
Thanks. Agreed. I've added the error logging.
There was a problem hiding this comment.
The exception message is now properly logged.
| std::size_t EventPool::num_free_events(CUcontext context) const | ||
| { | ||
| std::lock_guard const lock(_mutex); | ||
| auto it = _pools.find(context); | ||
| return (it != _pools.end()) ? it->second.size() : 0; | ||
| } | ||
|
|
||
| std::size_t EventPool::total_free_events() const |
There was a problem hiding this comment.
question: Do you need these for correct usage, or are they just going to be introspection facilities?
There was a problem hiding this comment.
They are just going to be introspection facilities. I plan to include them in the unit test.
There was a problem hiding this comment.
These utility functions are now being used in the unit test.
Co-authored-by: Lawrence Mitchell <wence@gmx.li>
KyleFromNVIDIA
left a comment
There was a problem hiding this comment.
Approved trivial CMake changes
madsbk
left a comment
There was a problem hiding this comment.
Looks good. Do you see any performance gain? Do you have a sense of the overhead of event creation/destruction?
| * https://docs.nvidia.com/cuda/cuda-programming-guide/02-basics/intro-to-cuda-cpp.html#runtime-initialization | ||
| * The OS reclaims process memory at exit regardless. | ||
| */ | ||
| class EventPool { |
There was a problem hiding this comment.
Consider naming them CudaEventPool and CudaEvent.
There was a problem hiding this comment.
Sure! Done!
|
|
||
| try { | ||
| std::lock_guard const lock(_mutex); | ||
| _pools[cuda_context].push_back(event); |
There was a problem hiding this comment.
This note may not hold for pooled events:
/**
* @note An event that has never been recorded reports `is_done() == true`,
* since CUDA's `cuEventQuery` returns `CUDA_SUCCESS` when there is no
* captured work.
*/If an Event goes out of scope without synchronize() being called, it is returned to the pool while its recorded work may still be pending on the stream. A subsequent pool.get() can then hand that same event to a new caller, for whom is_done() may report false even though they have never called record() themselves.
Either tighten the note to acknowledge that pooled events may retain pending work from a previous user, or make put() reject (or synchronize) non-idle events before returning them to the pool.
There was a problem hiding this comment.
That makes sense. I've updated the comment.
|
I haven't done a direct comparison between event pool vs on-the-fly event creation/destruction, but Mark (@harrism) did this benchmark before: https://github.com/harrism/cuda_event_benchmark |
madsbk
left a comment
There was a problem hiding this comment.
Looks good. Please report back when you get some performance numbers
|
Sure. Thanks for the review! |
|
/merge |
Summary
This PR adds a CUDA event pool, a thread-safe singleton that caches reusable CUDA events organized per CUDA context. This eliminates the per-acquisition
cuEventCreate/cuEventDestroycost in hot paths that need short-lived events (e.g. bounce-buffer recycling).This PR facilitates the implementation of device buffer support for the remote I/O multiplexing backend.
Related PR
Depends on #917
Addresses part of #914