Replace OwningResourceAdaptor with a BackRef mixin - #1078
Conversation
Signed-off-by: niranda perera <niranda.perera@gmail.com>
OwningResourceAdaptor with a BackRef mixin
Signed-off-by: niranda perera <niranda.perera@gmail.com>
Signed-off-by: niranda perera <niranda.perera@gmail.com>
|
Thanks @nirandaperera, I think this is a good idea. My main motivation for a generic wrapper like Before we go deeper into this PR, could you also implement, or at least sketch, the same pattern for |
|
I already added the Pinned MR and Host MR changes in this PR. Is that what you asked @madsbk ? Or did you want to sketch the owning wrapper impl for them? |
Yes, but on second thought, I think the trade-off is already pretty clear. It really comes down to one thing:
And I agree that the mixin approach is probably preferable overall. |
I agree. Even I didnt realize this resource_cast bug when I reviewed |
wence-
left a comment
There was a problem hiding this comment.
The core idea looks good, please apply some sanity to all of the docstrings and comments
Signed-off-by: niranda perera <niranda.perera@gmail.com>
Signed-off-by: niranda perera <niranda.perera@gmail.com>
Co-authored-by: Mads R. B. Kristensen <madsbk@gmail.com>
Signed-off-by: niranda perera <niranda.perera@gmail.com>
Signed-off-by: niranda perera <niranda.perera@gmail.com>
Signed-off-by: niranda perera <niranda.perera@gmail.com>
Signed-off-by: niranda perera <niranda.perera@gmail.com>
| @@ -205,6 +218,7 @@ class BufferResource : public std::enable_shared_from_this<BufferResource> { | |||
| * @throws std::invalid_argument if no pinned memory resource is available. | |||
| * @return Reference to the RMM resource used for pinned host allocations. | |||
| */ | |||
| // TODO: returned ref will not keep the BufferResource alive | |||
There was a problem hiding this comment.
Something like:
/**
* @brief Get the RMM pinned host memory resource.
*
* The returned reference does not by itself maintain the lifetime of this
* BufferResource. See device_mr() for details.
*
* @throws std::invalid_argument if no pinned memory resource is available.
* @return Reference to the RMM resource used for pinned host allocations.
*/
jameslamb
left a comment
There was a problem hiding this comment.
Seems like this PR is still under active development and discussion. Please @ me for a ci-codeowners approval once you're ready for that.
Signed-off-by: niranda perera <niranda.perera@gmail.com>
| * - **Uninstalled** (default-constructed): the instance is not bound to any | ||
| * owner. Copying an uninstalled instance throws `std::bad_weak_ptr`; a | ||
| * back-reference must be installed via `set_backref()` before copying. | ||
| * - **Installed** (after `set_backref()`): the instance is bound to a | ||
| * specific owner. Each copy of an installed instance acquires shared | ||
| * ownership of that owner for the lifetime of the copy. If the owner has | ||
| * been destroyed before the copy is made, copying throws | ||
| * `std::bad_weak_ptr`. |
There was a problem hiding this comment.
Please fix this description. How can an instance be bound to a specific owner and then the instance obtains via copy shared ownership of that owner?
How is the owner, who is the ownee?
There was a problem hiding this comment.
Ah my bad! semantics changed since I made it mandatory to set a backref before copying, So, uninstalled state is no longer valid.
| * Move operations transfer state without re-acquiring ownership. Equality | ||
| * is owner-based: two instances compare equal iff they reference the same | ||
| * owner, or are both uninstalled. |
There was a problem hiding this comment.
Why do I care about equality at all?
There was a problem hiding this comment.
Two back_ref objects will be equal if their weak ptrs point to the same control block. We cant simply use this == addressof(other) here.
There was a problem hiding this comment.
OK, but what do I need equality for? What goes wrong if (for example) we delete operator==
There was a problem hiding this comment.
I think for current usages, we dont need it, because rmm resource adapter defines its own equality operator. But I wanted to preserve the default equlity operator behavior.
Eg:
class Foo: BackRefMixin<int>{
int a;
bool operator==(const Foo&) const = default;
}I think for this to work, we need BackRefMixin to define an equality opertor.
|
Please also apply some work to fix the PR description. |
Signed-off-by: niranda perera <niranda.perera@gmail.com>
wence-
left a comment
There was a problem hiding this comment.
Some minor documentation suggestions
Signed-off-by: niranda perera <niranda.perera@gmail.com>
|
@jameslamb This is ready |
Signed-off-by: niranda perera <niranda.perera@gmail.com>
jameslamb
left a comment
There was a problem hiding this comment.
Believe this no longer needs ci-codeowners / packaging-codeowners but approving for you anyway.
I skimmed the changes and don't see any issues, deferring to the much more thorough and better-informed reviews given by other reviewers.
|
/merge |
Reverts #1078 Authors: - Niranda Perera (https://github.com/nirandaperera) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) URL: #1107
Take 2 of #1078 Authors: - Niranda Perera (https://github.com/nirandaperera) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) URL: #1108
… resources (#1106) `HostMemoryResource` and `PinnedMemoryResource` are now **`BufferResource`-only** resources that carry a strict `BackRefMixin<BufferResource>` back-reference. When a copy of one of these resources is made (e.g. when CCCL promotes `host_mr()` / `pinned_mr()` into an owning `cuda::mr::any_resource` inside a `HostBuffer`), the copy promotes the stored `weak_ptr` to a `shared_ptr`, keeping the owning `BufferResource` alive for as long as any derived buffer lives. This closes a lifetime gap where a `Buffer` could outlive the `BufferResource` that produced its memory resource. The mixin is *strict*: copying an instance without an installed back-reference throws `std::bad_weak_ptr`. To make that safe, these resources can no longer be constructed as standalone objects — they are created only by `BufferResource`, which installs the back-reference immediately after construction. This mirrors how `RmmResourceAdaptor` already works on this branch. ## C++ changes - **`HostMemoryResource`**: inherits `BackRefMixin<BufferResource>`; constructor is private with `friend class BufferResource`. Equality remains stateless (`always true`) since instances are interchangeable and the back-reference is installed exactly once. - **`PinnedMemoryResource`**: inherits `BackRefMixin<BufferResource>` as a second base; constructor is private (`friend class BufferResource`). The `make_if_available` / `from_options` factories were **removed** so no back-reference-less instance can ever escape. Equality compares the shared pool state only. - **`PinnedPoolProperties`**: gained a `numa_id` field (defaults to the calling thread's NUMA node) so all pinned configuration flows as one struct. Added a free helper `pinned_pool_properties_from_options(...)`. - **`PinnedMemoryDisabled`**: new `constexpr std::nullopt_t` sentinel used to disable pinned host memory (replaces the old `PinnedMemoryResource::Disabled`). - **`BufferResource::create()` / `from_options()`**: now take `std::optional<PinnedPoolProperties> pinned_pool_properties` (default `PinnedMemoryDisabled`) instead of a pre-built resource. The pinned resource is constructed internally and the back-reference is installed on `owning_mr_`, `host_mr_`, and `pinned_mr_` before `create()` returns. - **`try_pinned_mr()`**: now returns `std::optional<PinnedMemoryResource>` (a back-referenced handle) so Python/Statistics get a concrete, lifetime-safe handle. ## Python changes - **`PinnedMemoryResource`**: now an opaque, non-constructible handle (`__init__` raises `TypeError`); obtained via `BufferResource.pinned_mr`. - **`PinnedPoolProperties`**: new `@dataclass` (`initial_pool_size`, `max_pool_size`, `numa_id`) used to configure pinned memory. - **`BufferResource(...)`**: replaces the old `pinned_mr=` argument with `pinned_pool_properties: PinnedPoolProperties | None = None` (`None` disables pinned host memory). `from_options` derives this from the config options. - **`Statistics.report(pinned_mr=...)`**: unchanged signature; the handle is now sourced from `BufferResource.pinned_mr`. ## Tests - Added `HostMrKeepsBufferResourceAlive` and `PinnedMrKeepsBufferResourceAlive` regression tests mirroring `DeviceMrKeepsBufferResourceAlive`. - Routed all C++ tests/benchmarks through a `BufferResource` (no standalone Host/Pinned MR construction); reworked `test_host_buffer`, `test_config`, `test_memory_resources`, and `bench_memory_resources`. - Updated Python `test_config.py` to configure pinned memory via `BufferResource` and assert on `BufferResource.pinned_mr`. ## Breaking changes - C++: `HostMemoryResource` / `PinnedMemoryResource` can no longer be constructed directly; `PinnedMemoryResource::make_if_available` / `from_options` and the `Disabled` sentinel are removed. `BufferResource::create()` takes `PinnedPoolProperties` instead of a `PinnedMemoryResource`. - Python: `PinnedMemoryResource(...)` is no longer constructible; the `BufferResource(pinned_mr=...)` argument is replaced by `pinned_pool_properties=`. Depends on #1078 Closes #1070 Authors: - Niranda Perera (https://github.com/nirandaperera) Approvers: - Mads R. B. Kristensen (https://github.com/madsbk) URL: #1106
Replaces the
OwningResourceAdaptor<Resource, BackRef>wrapper with a slimBackRefMixin<BackRef>thatRmmResourceAdaptorinherits directly.OwningResourceAdaptorwrappedRmmResourceAdaptoronly to carry aweak_ptr/shared_ptr<BackRef>pair and promote weak→strong on copy. Itsallocate/deallocate/property forwarding was redundant sinceRmmResourceAdaptoralready satisfies the CCCL resource concept.Back-reference contract
BackRefMixinenforces a strict lifetime contract: a back-reference must be installed viaset_backref()before an instance is copied. Copying an uninstalled (or installed-but-expired) instance throwsstd::bad_weak_ptr. This makes accidental lifetime mistakes loud instead of silently producing a copy that no longer keeps its owner alive.To make the contract impossible to violate by mistake,
RmmResourceAdaptor's primary-resource constructor is now private. OnlyBufferResource(afriend) can construct one, and it installs the back-reference before the adaptor becomes observable. External callers obtain an adaptor exclusively viaBufferResource::device_mr_adaptor(), so every adaptor a caller can reach is guaranteed to be back-referenced and safely copyable.Changes
C++
cpp/include/rapidsmpf/memory/back_ref_mixin.hpp: templatedBackRefMixin<BackRef>with default-emptyweak_/strong_,set_backref(), copy ctor/assignment that promote weak→strong (throwingstd::bad_weak_ptrwhen uninstalled or expired), and owner-basedoperator==. The promote logic is inlined into the copy ctor/assignment (no helper).cpp/include/rapidsmpf/memory/owning_resource_adaptor.hpp.RmmResourceAdaptornow publicly inheritsBackRefMixin<BufferResource>. Its primary-resource constructor is private;BufferResourceis afriendand is the sole producer of new instances.operator==checks both shared state and back-reference identity.HostMemoryResourceandPinnedMemoryResourcedo not inherit the mixin (deferred); they remain freely copyable. Theiroperator==is pool-identity based (pinned) / always-equal (stateless host).BufferResource'sowning_mr_is a plainRmmResourceAdaptormember (nostd::optional).device_mris threaded through the private constructor soowning_mr_is initialized in the member-initializer list;BufferResource::create()installs the back-ref afterward viaowning_mr_.set_backref(weak_from_this())(device only).BufferResource::device_mr_adaptor(): exposes the internal deviceRmmResourceAdaptordirectly (e.g. to queryget_main_record()/current_allocated()), so callers no longer construct anRmmResourceAdaptorexternally.streaming::Context::from_options(...)first parameter changed fromRmmResourceAdaptor mrtoany_device_resource mr(a plain device MR), since external callers can no longer construct anRmmResourceAdaptor. TheContexthands the device MR to its internalBufferResource, which wraps it for tracking.BufferResource::create()/Context::from_options()and to read stats throughdevice_mr_adaptor()instead of creating and wrapping an adaptor themselves.Python bindings
RmmResourceAdaptor(Cython) now extendsDeviceMemoryResourceinstead ofUpstreamResourceAdaptor. Its__init__raisesTypeErrorto mirror the C++ private-constructor policy; instances are produced internally via a newcdef _from_cpp(const cpp_RmmResourceAdaptor&)factory that copies a back-ref'd C++ adaptor. Theget_upstreamaccessor is removed.BufferResource.device_mr_adaptor()(cpdef): returns a back-ref'dRmmResourceAdaptorwhose copies keep theBufferResourcealive. This is the only way to obtain an adaptor from Python.streaming.core.context.Context.from_options(...)now expectsmr: DeviceMemoryResourceinstead ofRmmResourceAdaptor(wrapped internally), matching the C++ change.Statistics.memory_profilingdocs/examples updated to obtain the adaptor viaBufferResource(...).device_mr_adaptor().RmmResourceAdaptor; pass plainrmm.mr.*resources toBufferResource/Context.from_optionsand read stats viabr.device_mr_adaptor().Latent side effect
Statistics::create_memory_recordercallscuda::mr::resource_cast<RmmResourceAdaptor>(&mr)on the resource passed in. Previously this cast failed for resources obtained fromBufferResource::device_mr()(the contained type wasOwningResourceAdaptor<…>), silently returning a no-opMemoryRecorder. After this PR the contained type isRmmResourceAdaptor, so the cast succeeds and memory profiling becomes active for BR-allocated memory. No tests were relying on the previous no-op behavior.