cuda.core: retain graph attachments with per-node user objects#2357
cuda.core: retain graph attachments with per-node user objects#2357Andy-Jost wants to merge 18 commits into
Conversation
Andy-Jost
added
P0
|
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. |
d95680d to
d7e8d3e
Compare
|
/ok to test |
d7e8d3e to
945672d
Compare
|
|
/ok to test |
807cac8 to
6d7ae74
Compare
6d7ae74 to
e7391d3
Compare
|
/ok to test |
e7391d3 to
8babae5
Compare
Preserve the validated lifetime and failure scenarios as executable specifications for the metadata redesign, alongside independent cleanup clarifications.
Establish canonical per-graph boxes, stable hierarchy ownership, and Level 1 graph identity before rebuilding attachment operations.
Give root and child graph handles stable per-graph boxes while sharing one hierarchy control block and registry identity.
Replace the mutable graph-wide slot table with complete per-node user objects and intrusive deferred cleanup.
Copy and rekey source attachment metadata into embedded graph hierarchies before publishing their canonical boxes.
Unregister and tombstone child graph boxes after CUDA destroys their owner while preserving stable storage for existing handles.
Allow graph-level anonymous attachments so a captured host callback stays safe when CUDA commits it but its node cannot be recovered.
Verify anonymous capture retention and proactive invalidation of child and conditional graph views.
Orient contributors to versioned node ownership, graph hierarchy metadata, clone propagation, invalidation, and deferred cleanup.
Make node identity atomic, preserve tombstoned graph identity, retry deferred cleanup safely, and cover final-reference launch ownership.
Document corrected graph attachment lifetime and callback-safe resource release.
Retain attachment owners and preallocate metadata before CUDA graph mutations so failures cannot leave nodes with unretained resources.
Carry rollback through the prepared attachment deleter so consumer extension modules do not depend on an out-of-line C++ symbol.
|
/ok to test |
8babae5 to
99333eb
Compare
Preserve the general no-invalidation guarantee while documenting borrowed child graphs as a narrow driver-owned exception, and tighten implementation comments for reviewers.
|
/ok to test |
mdboom
left a comment
mdboom
left a comment
There was a problem hiding this comment.
I have some clarifying questions about the design doc. Once I get my head around that I'll come back and look more closely at the code.
| The Cython graph code creates CUDA nodes and then calls these operations to keep | ||
| the C++ metadata synchronized with the driver. | ||
|
|
||
| ## Common operations |
There was a problem hiding this comment.
Can we put some of this concrete procedural content as comments next to the relevant code? It's sort of something you need to flip back-and-forth between to understand...
There was a problem hiding this comment.
Good call. I removed this stuff, except for the section on deferred cleanup. Key procedural details were moved into brief code comments. The document is overall much shorter and more focused.
| CUDA user objects tie externally managed resource lifetimes to CUDA graphs. | ||
| cuda-core uses them to keep resources referenced by graph node parameters | ||
| alive. It also keeps non-owning metadata that maps each node to the | ||
| user-object attachment backing its current parameters. This mapping lets | ||
| cuda-core update the correct user-object references during graph changes and | ||
| reproduce the node associations in graph clones. |
There was a problem hiding this comment.
I feel like the motivation is missing here, and it jumps right into the solution.
The motivation is that we must keep all resources used by graph parameters alive for the lifetime of any graph that may use them.
The solution, is therefore to use CUDA user objects as a way to keep user resources alive.
Do I have that right?
There was a problem hiding this comment.
A mermaid graph would go a long way here, showing all of the objects and how the relate with owning and non-owning pointers.
There was a problem hiding this comment.
Added a new section to address this. I also added a simple Mermaid diagram showing the Hierarchy/Box/Handle relationship and cleaned up the text in that section.
| A definition can be cloned, embedded as a child graph, or instantiated as a | ||
| `CUgraphExec`. Each operation copies the definition's current nodes and | ||
| parameters. The source and copy then have independent lifetimes: changing the | ||
| source does not change an existing clone or executable. An embedded clone is | ||
| owned by its parent node and is destroyed when that node is removed or | ||
| replaced. |
There was a problem hiding this comment.
Are we talking about definition in an abstract sense here, or CUgraph specifically? If the latter, maybe:
| A definition can be cloned, embedded as a child graph, or instantiated as a | |
| `CUgraphExec`. Each operation copies the definition's current nodes and | |
| parameters. The source and copy then have independent lifetimes: changing the | |
| source does not change an existing clone or executable. An embedded clone is | |
| owned by its parent node and is destroyed when that node is removed or | |
| replaced. | |
| A `CUgraph` can be cloned, embedded as a child graph, or instantiated as a | |
| `CUgraphExec`. Each operation copies the definition's current nodes and | |
| parameters. The source and copy then have independent lifetimes: changing the | |
| source does not change an existing clone or executable. An embedded clone is | |
| owned by its parent node and is destroyed when that node is removed or | |
| replaced. |
| Several versions of one logical node's parameters may therefore remain live at | ||
| the same time. Without version-specific ownership, replacing or deleting a node | ||
| could release memory, kernels, callbacks, or other resources that an existing | ||
| clone, executable, or launch still uses. |
There was a problem hiding this comment.
What is meant by "version" here? Isn't it just different "instances"? If not, maybe define "version" here.
In compiler runtimes, versioned data structures are usually used for caching -- you can compare a version number on a dictionary, for example, to see if it's been mutated since the last time you cached something about it. I don't see anything like that happening in the code (unless I missed it), so I feel like "instance" is a more precise term.
There was a problem hiding this comment.
I added an introduction that should clarify
| clone, executable, or launch still uses. | ||
|
|
||
| cuda-core prevents this by giving each parameter version an immutable | ||
| `NodeAttachment` owned through a graph-retained CUDA user object. Non-owning |
There was a problem hiding this comment.
For clarity -- this is immutable on the C++ side, but may reference mutable data on the Python side (most Python data is). I don't think that matters to the implementation, I just found it confusing as to what layer is being talked about.
There was a problem hiding this comment.
Clarified to
a
NodeAttachmentwhose owner bundle is immutable after publication (although the resources it keeps alive may themselves be mutable).
Transitive mutability is a real problem without a good solution I can see. Example: someone could embed a cuda.core.Buffer in a kernel parameter bundle and then close the buffer.
| `GraphHierarchy::graveyard`. Keeping this tombstone at a stable address lets | ||
| existing aliasing handles safely observe that the graph is invalid. Removing |
There was a problem hiding this comment.
Where does this happen? I only see graphs being put in the graveyard. Must be something more subtle going on?
There was a problem hiding this comment.
This occurs in invalidate_child_graph_state. The object moved to the graveyard is a GraphBox, which contains the destroyed child graph’s CUgraph and related state. Each GraphHandle aliases that CUgraph field while sharing ownership of the containing GraphHierarchy.
When a parent node containing a child graph is destroyed, CUDA also destroys the embedded child graph, invalidating its raw handle. This PR sets the GraphBox’s CUgraph field to null so existing aliases reflect that invalidation, then moves the retired box to the graveyard so the aliased field’s address remains valid.
This machinery is necessary because CUDA couples removal of an embedded child graph from its parent with destruction of that graph. If the API allowed the graph to be detached and its ownership transferred to the caller, cuda.core could use an ordinary owning handle and call cuGraphDestroy after its final reference was released.
| ----- | ||
| Definitions that view the same root, child, or conditional graph hierarchy | ||
| share underlying graph state. Mutations anywhere in that hierarchy must be | ||
| externally synchronized. |
There was a problem hiding this comment.
The only real issue my agent found was that self-referencing a graph may create a cycle that the Python garbage collector may be unable to break. We could add a comment to that effect, but I sort of think it seems very unlikely someone would do that.
There was a problem hiding this comment.
Thanks for pointing this out. The general problem is that Python's cyclic garbage collector cannot trace references through the driver, so reference cycles involving retained user objects result in leaks. This limitation is very difficult to remove, so for this PR I opted to update the docs.
I noted the limitation in GRAPH_ATTACHMENTS.md and the docstrings (example here) for API functions that retain Python objects on graphs:
• GraphNode.callback and GraphBuilder.callback
• GraphNode.launch for retained kernel arguments
• GraphNode.memset and GraphNode.memcpy for retained buffers and explicit owners
Reframe the design around parameter versions and driver-managed lifetimes, and keep mutation ordering guidance beside the implementation.
Replace the ASCII sketch with Mermaid and illustrate the GraphHandle alias and control-block relationships for reviewers.
Document that driver-held Python references are opaque to cyclic GC and identify affected graph APIs.
2 participants
Summary
Fix the graph attachment lifetime problem described in #2350. Each resource-bearing node version now has an immutable attachment owned through its own CUDA user object, so source-graph mutation cannot release resources still used by clones, executable graphs, or in-flight launches.
Implementing that ownership change also required cuda-core's metadata to follow CUDA's graph topology and mutation boundaries. Each graph hierarchy keeps root, child, and conditional graph state coherent; clone and embed paths copy and rekey that metadata, and prepare/commit operations publish it only after the corresponding CUDA mutation succeeds. Together, these pieces make cuda-core's bookkeeping match the lifetimes CUDA actually enforces.
Changes
Review guide
cuda_core/cuda/core/_cpp/GRAPH_ATTACHMENTS.mdfor the ownership model, graph hierarchy, common operations, and invariants.resource_handles.cpp/.hppfor per-node user objects, canonical graph boxes, clone metadata import, handle invalidation, and deferred cleanup._resource_handles.*andgraph/*.pyxfor the Cython integration and CUDA-operation ordering.Test coverage
pre-commit run --all-filesRelated Work