[libcu++] Add shared versions of memory pool owning objects#8802
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| operator!=(const __shared_block_ptr& __lhs, const __shared_block_ptr& __rhs) noexcept | ||
| { | ||
| return __lhs.__block_ != __rhs.__block_; | ||
| } |
There was a problem hiding this comment.
Important: shared_ptr and friends also implement the other relational operators. We probably don't need all of them, but at the very least we should:
- Implement operators against
nullptr_t, so we can say__shard_block_ptr == nullptr - Implement
operator<so we can put this instd::map - Implement
std::hashso we can put this in astd::unordered_map
There was a problem hiding this comment.
This an internal type, I would rather add that stuff on case by case basis when its needed
| _CCCL_HOST_API shared_resource(const shared_resource& __other) noexcept | ||
| : __block_(__other.__block_) | ||
| {} |
There was a problem hiding this comment.
Important: can be = default since __block_ implements copy constructors.
There was a problem hiding this comment.
If I do it nvcc 12 gets confused in execution space checks
| _CCCL_HOST_API shared_resource(shared_resource&& __other) noexcept | ||
| : __block_(::cuda::std::move(__other.__block_)) | ||
| {} |
There was a problem hiding this comment.
Important: can be = default as well.
There was a problem hiding this comment.
If I do it nvcc 12 gets confused in execution space checks
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bdice
left a comment
bdice
left a comment
There was a problem hiding this comment.
Generally looks good to me. I had one question about the supported CUDA version for pinned pools. I am okay with requiring 12.9 -- we already require 12.9 for all CUDA 12 compilation in RAPIDS -- just wanted to verify the intent and make sure I wasn't missing something.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis PR introduces shared-ownership memory pools by implementing a ref-counted smart-pointer infrastructure, updating the existing shared_resource type to use it, extending standalone pools with empty-construction and ownership-transfer APIs, and adding three new shared pool variants for device, managed, and pinned memory with automatic pool destruction on scope exit. ChangesShared-ownership memory pool infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
libcudacxx/include/cuda/__memory_resource/shared_block_ptr.h (1)
67-68: 💤 Low valueAdd
noexceptto the default constructor.The default constructor only default-initializes
__block_tonullptr, which cannot throw.Suggested fix
//! `@brief` Constructs a null ``__shared_block_ptr`` with no control block. - __shared_block_ptr() = default; + __shared_block_ptr() noexcept = default;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libcudacxx/include/cuda/__memory_resource/shared_block_ptr.h` around lines 67 - 68, The default constructor __shared_block_ptr() should be marked noexcept because it only initializes __block_ to nullptr; update the declaration of __shared_block_ptr() to be __shared_block_ptr() noexcept = default; so the constructor is noexcept-qualified (no other code changes needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libcudacxx/include/cuda/__memory_pool/shared_managed_memory_pool.h`:
- Around line 73-78: Add the [[nodiscard]] attribute to the static factory
shared_managed_memory_pool::from_native_handle and extend its Doxygen comment to
include a //! `@return` describing that it returns a shared_managed_memory_pool
which takes shared ownership of the provided ::cudaMemPool_t handle; keep the
function signature otherwise unchanged and ensure the documentation clearly
states ownership semantics.
In `@libcudacxx/include/cuda/__memory_pool/shared_memory_pool_base.h`:
- Around line 97-101: The move constructor
__shared_memory_pool_base(__shared_memory_pool_base&& __other) leaves
__other.__pool_ unchanged which is misleading because __other.__ref_ no longer
owns the control block; update the move ctor (the function in the
__shared_memory_pool_base class that delegates to __memory_pool_base and moves
__ref_) to explicitly reset __other.__pool_ (e.g., set it to nullptr or
equivalent empty handle) after moving __ref_ so the moved-from object's state is
consistent and __other.get() no longer returns a valid-looking handle.
In `@libcudacxx/include/cuda/__memory_pool/shared_pinned_memory_pool.h`:
- Around line 84-89: Add the [[nodiscard]] attribute to the static factory
shared_pinned_memory_pool::from_native_handle(::cudaMemPool_t __pool) and update
its Doxygen to include a //! `@return` describing that it returns a
shared_pinned_memory_pool that takes shared ownership of the provided
::cudaMemPool_t; keep the function noexcept and implementation returning
shared_pinned_memory_pool(__pool).
---
Nitpick comments:
In `@libcudacxx/include/cuda/__memory_resource/shared_block_ptr.h`:
- Around line 67-68: The default constructor __shared_block_ptr() should be
marked noexcept because it only initializes __block_ to nullptr; update the
declaration of __shared_block_ptr() to be __shared_block_ptr() noexcept =
default; so the constructor is noexcept-qualified (no other code changes
needed).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 1e78e6a6-0ccd-4859-bdcd-01169cc19c3f
📒 Files selected for processing (13)
libcudacxx/include/cuda/__memory_pool/device_memory_pool.hlibcudacxx/include/cuda/__memory_pool/managed_memory_pool.hlibcudacxx/include/cuda/__memory_pool/memory_pool_base.hlibcudacxx/include/cuda/__memory_pool/pinned_memory_pool.hlibcudacxx/include/cuda/__memory_pool/shared_device_memory_pool.hlibcudacxx/include/cuda/__memory_pool/shared_managed_memory_pool.hlibcudacxx/include/cuda/__memory_pool/shared_memory_pool_base.hlibcudacxx/include/cuda/__memory_pool/shared_pinned_memory_pool.hlibcudacxx/include/cuda/__memory_resource/shared_block_ptr.hlibcudacxx/include/cuda/__memory_resource/shared_resource.hlibcudacxx/include/cuda/memory_poollibcudacxx/test/libcudacxx/cuda/memory_resource/resources/memory_pools.culibcudacxx/test/libcudacxx/cuda/memory_resource/resources/shared_memory_pools.cu
💤 Files with no reviewable changes (1)
- libcudacxx/include/cuda/__memory_pool/memory_pool_base.h
| //! @brief Constructs a shared managed memory pool from an existing native handle. | ||
| //! @param __pool The ``cudaMemPool_t`` to take shared ownership of. | ||
| _CCCL_HOST_API static shared_managed_memory_pool from_native_handle(::cudaMemPool_t __pool) noexcept | ||
| { | ||
| return shared_managed_memory_pool(__pool); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add [[nodiscard]] and complete Doxygen documentation.
The from_native_handle static factory has two issues:
- Missing
[[nodiscard]]attribute — the function returns a value with no side effects, so discarding the result is likely a mistake. - Incomplete Doxygen documentation — missing
//!@return`` describing the returned object.
As per coding guidelines: "Most functions with a non-void return type shall use [[nodiscard]]" and "When a function is documented with Doxygen, it must include... //! @return`` for non-void functions".
Proposed fix
//! `@brief` Constructs a shared managed memory pool from an existing native handle.
//! `@param` __pool The ``cudaMemPool_t`` to take shared ownership of.
- _CCCL_HOST_API static shared_managed_memory_pool from_native_handle(::cudaMemPool_t __pool) noexcept
+ //! `@return` A shared_managed_memory_pool that shares ownership of the provided handle.
+ _CCCL_NODISCARD _CCCL_HOST_API static shared_managed_memory_pool from_native_handle(::cudaMemPool_t __pool) noexcept
{
return shared_managed_memory_pool(__pool);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libcudacxx/include/cuda/__memory_pool/shared_managed_memory_pool.h` around
lines 73 - 78, Add the [[nodiscard]] attribute to the static factory
shared_managed_memory_pool::from_native_handle and extend its Doxygen comment to
include a //! `@return` describing that it returns a shared_managed_memory_pool
which takes shared ownership of the provided ::cudaMemPool_t handle; keep the
function signature otherwise unchanged and ensure the documentation clearly
states ownership semantics.
| _CCCL_EXEC_CHECK_DISABLE | ||
| _CCCL_HOST_API __shared_memory_pool_base(__shared_memory_pool_base&& __other) noexcept | ||
| : __memory_pool_base(__other.__pool_) | ||
| , __ref_(::cuda::std::move(__other.__ref_)) | ||
| {} |
There was a problem hiding this comment.
Move constructor leaves __other.__pool_ unchanged.
After moving, __other.__ref_ no longer owns the control block, but __other.__pool_ still holds the raw pool handle. While this doesn't cause a double-free (the __pool_destroyer in the moved-from __ref_ won't destroy anything), it leaves __other in a potentially confusing state where __other.get() returns a valid-looking handle that the object no longer owns.
Consider nulling out __other.__pool_ for consistency with the moved-from state of the reference:
Suggested fix
_CCCL_EXEC_CHECK_DISABLE
_CCCL_HOST_API __shared_memory_pool_base(__shared_memory_pool_base&& __other) noexcept
- : __memory_pool_base(__other.__pool_)
+ : __memory_pool_base(::cuda::std::exchange(__other.__pool_, nullptr))
, __ref_(::cuda::std::move(__other.__ref_))
{}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _CCCL_EXEC_CHECK_DISABLE | |
| _CCCL_HOST_API __shared_memory_pool_base(__shared_memory_pool_base&& __other) noexcept | |
| : __memory_pool_base(__other.__pool_) | |
| , __ref_(::cuda::std::move(__other.__ref_)) | |
| {} | |
| _CCCL_EXEC_CHECK_DISABLE | |
| _CCCL_HOST_API __shared_memory_pool_base(__shared_memory_pool_base&& __other) noexcept | |
| : __memory_pool_base(::cuda::std::exchange(__other.__pool_, nullptr)) | |
| , __ref_(::cuda::std::move(__other.__ref_)) | |
| {} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libcudacxx/include/cuda/__memory_pool/shared_memory_pool_base.h` around lines
97 - 101, The move constructor
__shared_memory_pool_base(__shared_memory_pool_base&& __other) leaves
__other.__pool_ unchanged which is misleading because __other.__ref_ no longer
owns the control block; update the move ctor (the function in the
__shared_memory_pool_base class that delegates to __memory_pool_base and moves
__ref_) to explicitly reset __other.__pool_ (e.g., set it to nullptr or
equivalent empty handle) after moving __ref_ so the moved-from object's state is
consistent and __other.get() no longer returns a valid-looking handle.
| //! @brief Constructs a shared pinned memory pool from an existing native handle. | ||
| //! @param __pool The ``cudaMemPool_t`` to take shared ownership of. | ||
| _CCCL_HOST_API static shared_pinned_memory_pool from_native_handle(::cudaMemPool_t __pool) noexcept | ||
| { | ||
| return shared_pinned_memory_pool(__pool); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add [[nodiscard]] and complete Doxygen documentation.
The from_native_handle static factory has two issues:
- Missing
[[nodiscard]]attribute — the function returns a value with no side effects, so discarding the result is likely a mistake. - Incomplete Doxygen documentation — missing
//!@return`` describing the returned object.
As per coding guidelines: "Most functions with a non-void return type shall use [[nodiscard]]" and "When a function is documented with Doxygen, it must include... //! @return`` for non-void functions".
Proposed fix
//! `@brief` Constructs a shared pinned memory pool from an existing native handle.
//! `@param` __pool The ``cudaMemPool_t`` to take shared ownership of.
- _CCCL_HOST_API static shared_pinned_memory_pool from_native_handle(::cudaMemPool_t __pool) noexcept
+ //! `@return` A shared_pinned_memory_pool that shares ownership of the provided handle.
+ _CCCL_NODISCARD _CCCL_HOST_API static shared_pinned_memory_pool from_native_handle(::cudaMemPool_t __pool) noexcept
{
return shared_pinned_memory_pool(__pool);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libcudacxx/include/cuda/__memory_pool/shared_pinned_memory_pool.h` around
lines 84 - 89, Add the [[nodiscard]] attribute to the static factory
shared_pinned_memory_pool::from_native_handle(::cudaMemPool_t __pool) and update
its Doxygen to include a //! `@return` describing that it returns a
shared_pinned_memory_pool that takes shared ownership of the provided
::cudaMemPool_t; keep the function noexcept and implementation returning
shared_pinned_memory_pool(__pool).
This comment has been minimized.
This comment has been minimized.
| : __memory_pool_base(__pool) | ||
| , __ref_(__pool) |
There was a problem hiding this comment.
We duplicate the stored handles? We store cudaMempool_t once in __memory_pool_base and the second time in __shared_block_ptr? Can't we do better?
There was a problem hiding this comment.
I implemented it first without the duplication, but it made the implementation much messier, I don't think it's worth it. You either use only the copy in the shared_block and then you have to reimplement half of __memory_pool_base or you only use the one in the pool base, but you have to do some weird destruction pattern, where you have an explicit destroy call for the shared_block. Both are workable solutions, but the code becomes either redundant or difficult to follow
🥳 CI Workflow Results🟩 Finished in 2h 12m: Pass: 100%/113 | Total: 3d 03h | Max: 1h 22m | Hits: 58%/627564See results here. |
4 participants
This PR adds
shared_*_memory_pooltypes, which are memory pools with shared ownership semantics. Currentlyshared_resourcecan be used with a memory pool type to have shared ownership of a pool, but it has some downsides:shared_resourcewrapper exist, if they just reach for the memory pool type it won't work out of the box withany_resource,bufferetc..res.get.member(), the new types have the members available immediatelyThis PR also removes
releasemember function from*_memory_pool_reftypes that was added there by mistake and didn't make much sense.It also adds
no_initconstructors to*_memory_pooltypes to align with the shared counterparts and provide a way to have an object you move a pool into at some later point.The implementation moved the ref counting bits of
shared_resourceto an internal class, so it could be reused for these new shared pool types. There is a bit of duplication because of that, the deleter holds another copy of the memory pool pointer, but it allowed to keep the implementation simplerSummary by CodeRabbit
Release Notes
New Features
shared_device_memory_pool,shared_managed_memory_pool, andshared_pinned_memory_poolenabling copyable shared ownership semanticsno_initconstructorrelease()method to existing pool types for explicit ownership transferTests