-
Notifications
You must be signed in to change notification settings - Fork 199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement cudax::shared_resource
#2398
Conversation
c624c70
to
1f76436
Compare
We currently have two basic building blocks around memory resources, `any_resource` and `resource_ref`. However, while they make owning and sharing resources much easier, we can still run into lifetime issues. If a user wants to pass a resource into a library function that might exceed the lifetime of the resource, they would need to move it into an any_resource. However, they also might want to share that resource among multiple functions, e.g a pool allocator. We need a way to properly share a resource in those circumstances. Enter `shared_resource`. Rather than storing an `any_resource` this holds a `shared_ptr<any_resource>`. With that we can happily copy / move them around and without touching the stored resource.
1f76436
to
5414633
Compare
i think this is the wrong design. |
This is not about copyability but the lifetime of the passed in resource. Currently we can either have reference semantics and have potential dangling references, or we can have ownership but no reference semantic. This is the combination of both. |
🟩 CI finished in 5h 12m: Pass: 100%/58 | Total: 2h 49m | Avg: 2m 55s | Max: 8m 00s | Hits: 85%/206
|
Project | |
---|---|
CCCL Infrastructure | |
libcu++ | |
CUB | |
Thrust | |
+/- | CUDA Experimental |
pycuda | |
CUDA C Core Library |
Modifications in project or dependencies?
Project | |
---|---|
CCCL Infrastructure | |
libcu++ | |
CUB | |
Thrust | |
+/- | CUDA Experimental |
pycuda | |
CUDA C Core Library |
🏃 Runner counts (total jobs: 58)
# | Runner |
---|---|
44 | linux-amd64-cpu16 |
8 | linux-amd64-gpu-v100-latest-1 |
4 | linux-arm64-cpu16 |
2 | windows-amd64-cpu16 |
cudax/include/cuda/experimental/__memory_resource/shared_resource.cuh
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think #2410 is a better way forward
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions, a couple of spelling errors (does CCCL have spell check for docs in CI?)
cudax/include/cuda/experimental/__memory_resource/shared_resource.cuh
Outdated
Show resolved
Hide resolved
cudax/include/cuda/experimental/__memory_resource/shared_resource.cuh
Outdated
Show resolved
Hide resolved
//! held by this \c shared_resource object is released, while the reference held both \c __other | ||
//! is transfered to this object. | ||
//! @param __other The \c shared_resource object to move from. | ||
/// @post \c __other is left in a valid but unspecified state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "valid but unspecified state" mean? Is it valid to use other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the common phrase of move semantics.
It means dont touch it
//! | ||
//! @endrst | ||
template <class... _Properties> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happened to properties? They enable interfaces to be defined that expect to take shared ownership of an MR with specific properties. I think we need shared_resource
to work like resource_ref
with respect to properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we do not need that anymore, because we are using the explicit _Resource
type to forward the properties
🟩 CI finished in 4h 52m: Pass: 100%/58 | Total: 2h 55m | Avg: 3m 01s | Max: 12m 29s | Hits: 83%/212
|
Project | |
---|---|
CCCL Infrastructure | |
libcu++ | |
CUB | |
Thrust | |
+/- | CUDA Experimental |
pycuda | |
CUDA C Core Library |
Modifications in project or dependencies?
Project | |
---|---|
CCCL Infrastructure | |
libcu++ | |
CUB | |
Thrust | |
+/- | CUDA Experimental |
pycuda | |
CUDA C Core Library |
🏃 Runner counts (total jobs: 58)
# | Runner |
---|---|
44 | linux-amd64-cpu16 |
8 | linux-amd64-gpu-v100-latest-1 |
4 | linux-arm64-cpu16 |
2 | windows-amd64-cpu16 |
Co-authored-by: Mark Harris <[email protected]>
🟩 CI finished in 1h 45m: Pass: 100%/58 | Total: 3h 04m | Avg: 3m 10s | Max: 11m 14s | Hits: 83%/212
|
Project | |
---|---|
CCCL Infrastructure | |
libcu++ | |
CUB | |
Thrust | |
+/- | CUDA Experimental |
pycuda | |
CUDA C Core Library |
Modifications in project or dependencies?
Project | |
---|---|
CCCL Infrastructure | |
libcu++ | |
CUB | |
Thrust | |
+/- | CUDA Experimental |
pycuda | |
CUDA C Core Library |
🏃 Runner counts (total jobs: 58)
# | Runner |
---|---|
44 | linux-amd64-cpu16 |
8 | linux-amd64-gpu-v100-latest-1 |
4 | linux-arm64-cpu16 |
2 | windows-amd64-cpu16 |
template <class _Resource, class... _Args> | ||
auto make_shared_resource(_Args&&... __args) -> shared_resource<_Resource> | ||
{ | ||
static_assert(_CUDA_VMR::resource<_Resource>, "_Resource does not satisfy the cuda::mr::resource concept"); | ||
return shared_resource<_Resource>{_CUDA_VSTD::forward<_Args>(__args)...}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is make_shared_resource<_Resource>(args...)
any different than shared_resource<_Resource>(args...)
apart from being five characters longer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it is just symmetric with all the other stuff
@miscco are you ok with this pr as it is now? |
We are going forward with merging this now and fix stuff when mark is back from vacation
There wasn't anything left to address in my review anyway, was there? |
We currently have two basic building blocks around memory resources,
any_resource
andresource_ref
.However, while they make owning and sharing resources much easier, we can still run into lifetime issues.
If a user wants to pass a resource into a library function that might exceed the lifetime of the resource, they would need to move it into an
any_resource
.any_resource
requires movability and copyability, and not all resource types are movable and copyable.However, they also might want to share that resource among multiple functions, e.g a pool allocator. We need a way to properly share a resource in those circumstances.
Enter
shared_resource
. Ashared_resource<_Resource>
behaves like ashared_ptr<_Resource>
while also satisfying theresource
concept. Users can construct their (potentially immovable) resource in ashared_resource
instance, and then move theshared_resource
into anany_resource
. Now all copies of thatany_resource
instance with share ownership of the same underlying resource.