Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add complete set of resource ref aliases #1479
Add complete set of resource ref aliases #1479
Changes from all commits
115f0d4
708abca
a101d05
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 it will be common for users of
async_resource_ref
for pinned host memory to also wantdevice_accessible
. (I expect you will want it in your cuDF usage eventually too...) My question is, do we want to just addcuda::mr::device_accessible
to this alias, or add another one:Thoughts @nvdbaranec @Missco @jrhemstad ?
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 would not add
device_accessible
to thehost_async_resource_ref
as that would all potential memory resource to satisfydevice_accessible
On the other hand an async resource has no real value without
device_accessible
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.
Good point. However I think an async resource ref that is host-only has value because it specifies that it will only be used on the host, but it may still be backed by pinned memory. And then it benefits from stream-ordered allocation (e.g. can be used in a stream-ordered pool) and also can be used as the host memory for SOL
cudaMemcpyAsync
.So really for completeness I think we probably need the full set:
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.
The use case for this right now in cudf is strictly in the
hostdevice_vector
class, whose sole purpose is to force explicit host<->device copying onto the user. That is: entirely avoiding the device touching pinned memory directly. So in that sense, stream ordering at the allocator level isn't necessary.https://github.com/rapidsai/cudf/blob/branch-24.04/cpp/src/io/utilities/hostdevice_vector.hpp
As for whether cudf at large will ever need this, I'm not sure. At the moment it definitely prefers to be explicit about all host<-> device handoffs.