Add a strict Buffer Resource back-reference to Host and Pinned memory resources - #1106
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. |
Signed-off-by: niranda perera <niranda.perera@gmail.com>
ebc6a70 to
ec86fe5
Compare
ec86fe5 to
2db591e
Compare
…t-pinned-mr Signed-off-by: niranda perera <niranda.perera@gmail.com>
2db591e to
dfd225d
Compare
Signed-off-by: niranda perera <niranda.perera@gmail.com>
wence-
left a comment
There was a problem hiding this comment.
Code changes broadly look good, but please clean up the docstrings and comments throughout.
In general, please try and elide as much implementation detail in comments and, if it is necessary, have it only in one place rather than everywhere. Docstrings and comments should generally talk about observable behaviour and semantics, not implementation specifics.
Co-authored-by: Mads R. B. Kristensen <madsbk@gmail.com> Co-authored-by: Lawrence Mitchell <wence@gmx.li>
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>
There was a problem hiding this comment.
Looks good. The only thing we should consider is the enablement semantics. Currently, setting pinned_pool_properties to a non-None value does not guarantee that pinned memory is enabled, since that still depends on system support.
It would be helpful to have an option that means: enable pinned memory, or raise an error if it is unavailable.
Alternatively, this option could be implemented at the cudf-polars level. After creating the memory resource, cudf-polars could verify that pinned memory was actually enabled and raise an error otherwise.
Signed-off-by: niranda perera <niranda.perera@gmail.com>
Signed-off-by: niranda perera <niranda.perera@gmail.com>
I made |
…ckref-host-pinned-mr Signed-off-by: niranda perera <niranda.perera@gmail.com>
|
/merge |
dismissing request as all comments have been addressed
## Description Adds PR changes to Host and Pinned MRs. Depends on rapidsai/rapidsmpf#1106 ## Checklist - [x] I am familiar with the [Contributing Guidelines](https://github.com/rapidsai/cudf/blob/HEAD/CONTRIBUTING.md). - [x] New or existing tests cover these changes. - [x] The documentation is up to date with these changes. --------- Signed-off-by: niranda perera <niranda.perera@gmail.com>
## Description Adds PR changes to Host and Pinned MRs. Depends on rapidsai/rapidsmpf#1106 ## Checklist - [x] I am familiar with the [Contributing Guidelines](https://github.com/rapidsai/cudf/blob/HEAD/CONTRIBUTING.md). - [x] New or existing tests cover these changes. - [x] The documentation is up to date with these changes. --------- Signed-off-by: niranda perera <niranda.perera@gmail.com>
HostMemoryResourceandPinnedMemoryResourceare nowBufferResource-only resources that carry a strictBackRefMixin<BufferResource>back-reference. When a copy of one of these resources is made (e.g. when CCCL promoteshost_mr()/pinned_mr()into an owningcuda::mr::any_resourceinside aHostBuffer), the copy promotes the storedweak_ptrto ashared_ptr, keeping the owningBufferResourcealive for as long as any derived buffer lives. This closes a lifetime gap where aBuffercould outlive theBufferResourcethat 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 byBufferResource, which installs the back-reference immediately after construction. This mirrors howRmmResourceAdaptoralready works on this branch.C++ changes
HostMemoryResource: inheritsBackRefMixin<BufferResource>; constructor is private withfriend class BufferResource. Equality remains stateless (always true) since instances are interchangeable and the back-reference is installed exactly once.PinnedMemoryResource: inheritsBackRefMixin<BufferResource>as a second base; constructor is private (friend class BufferResource). Themake_if_available/from_optionsfactories were removed so no back-reference-less instance can ever escape. Equality compares the shared pool state only.PinnedPoolProperties: gained anuma_idfield (defaults to the calling thread's NUMA node) so all pinned configuration flows as one struct. Added a free helperpinned_pool_properties_from_options(...).PinnedMemoryDisabled: newconstexpr std::nullopt_tsentinel used to disable pinned host memory (replaces the oldPinnedMemoryResource::Disabled).BufferResource::create()/from_options(): now takestd::optional<PinnedPoolProperties> pinned_pool_properties(defaultPinnedMemoryDisabled) instead of a pre-built resource. The pinned resource is constructed internally and the back-reference is installed onowning_mr_,host_mr_, andpinned_mr_beforecreate()returns.try_pinned_mr(): now returnsstd::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__raisesTypeError); obtained viaBufferResource.pinned_mr.PinnedPoolProperties: new@dataclass(initial_pool_size,max_pool_size,numa_id) used to configure pinned memory.BufferResource(...): replaces the oldpinned_mr=argument withpinned_pool_properties: PinnedPoolProperties | None = None(Nonedisables pinned host memory).from_optionsderives this from the config options.Statistics.report(pinned_mr=...): unchanged signature; the handle is now sourced fromBufferResource.pinned_mr.Tests
HostMrKeepsBufferResourceAliveandPinnedMrKeepsBufferResourceAliveregression tests mirroringDeviceMrKeepsBufferResourceAlive.BufferResource(no standalone Host/Pinned MR construction); reworkedtest_host_buffer,test_config,test_memory_resources, andbench_memory_resources.test_config.pyto configure pinned memory viaBufferResourceand assert onBufferResource.pinned_mr.Breaking changes
HostMemoryResource/PinnedMemoryResourcecan no longer be constructed directly;PinnedMemoryResource::make_if_available/from_optionsand theDisabledsentinel are removed.BufferResource::create()takesPinnedPoolPropertiesinstead of aPinnedMemoryResource.PinnedMemoryResource(...)is no longer constructible; theBufferResource(pinned_mr=...)argument is replaced bypinned_pool_properties=.Depends on #1078
Closes #1070