cuda.core: defer graph user-object payload cleanup#2371
Conversation
Move graph attachment destruction off CUDA's internal callback thread through a coalesced Py_AddPendingCall drain. Queue payloads lock-free, retry scheduling failures from later safe entries, and leak intact payloads once Python finalization begins.
Verify more than 32 CUDA callbacks coalesce onto Python's main thread, recover after Py_AddPendingCall queue saturation, and remain safe during interpreter shutdown.
|
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. |
|
PR #2371 separates “CUDA says this payload is no longer needed” from actually cleaning it up. Parts1.
|
Add concise descriptions for each reclaimer operation and state field, and explain the typed synchronous helper versus CUDA's void-pointer callback ABI.
70d601a to
f454e13
Compare
Use the process-wide reclaimer directly instead of storing the same pointer in every envelope, publish the singleton atomically, and make the intrusive next link non-atomic under the queue's release/acquire handoff.
Use payload, cleanup item, cleanup queue, enqueue, drain, and cleanup consistently across implementation, documentation, and tests.
Run the interpreter-shutdown probe outside the source tree so CI imports the installed package with generated modules.
This comment has been minimized.
This comment has been minimized.
Validate queue-saturation safety without assuming when CUDA invokes its asynchronous user-object destructor.
mdboom
left a comment
There was a problem hiding this comment.
My initial reaction was that this might be more complex than it needs to be. Py_AddPendingCall already functions as a queue (you can call it multiple times and it will run all of the calls in order the next chance it calls any of them), so it might not be necessary to also have a queue on our side.
/However/ Py_AddPendingCall is a FIFO queue, with a maximum of 300 elements (and it appears to silently evict elements if it's more than 300). All that is "probably fine", but it's an implementation detail that could change.
So I think what's here is preferable -- we can be in charge of our own destiny on the queueing. (Say, for example, we discover that the ordering needs to change, this gives us a place to do so).
My other comments are just around improving comments and docs.
| auto* queue = new DeferredCleanupQueue(); | ||
| deferred_cleanup_queue.store(queue, std::memory_order_release); | ||
| if (Py_AtExit(stop_deferred_cleanup) != 0) { | ||
| queue->stop(); | ||
| } |
There was a problem hiding this comment.
This is thread-safe only as long as the GIL is held -- it is in practice (also it's run once at import under normal conditions) but maybe just add a comment.
| void retry_schedule() noexcept { | ||
| schedule(); | ||
| } |
There was a problem hiding this comment.
Maybe add a little context about why retrying would be necessary? I don't think it's necessarily unsound, just could use a little motivating statement.
|
Thanks for the review, @mdboom. I'll wrap the touch-ups into the next PR (#2357). As for the complexity, I largely agree. My initial idea was very similar to yours: simply make a call to
So I agree, the complexity is higher than anticipated, but I don't think it can be reduced. Queue limit details
|
|
Summary
Move graph attachment payload destruction off CUDA's internal user-object callback thread. The callback now performs only an intrusive queue handoff and one coalesced
Py_AddPendingCall; resource handles and Python owners are released later from Python's main thread, where their deleters may safely call CUDA.Changes
Related Work