-
Notifications
You must be signed in to change notification settings - Fork 32
Use cudf::pack with pinned mr in TableChunk::copy
#966
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
base: main
Are you sure you want to change the base?
Changes from 7 commits
d7051d4
a2a27c9
3e593a0
b4fc954
9412778
0b00c6e
634c903
6371c76
5fe889b
7769d1f
53d7fe7
aa7668d
a01e7d1
5a9ef07
10c2a53
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,7 +82,7 @@ rmm::host_async_resource_ref BufferResource::host_mr() noexcept { | |
| return host_mr_; | ||
| } | ||
|
|
||
| rmm::host_async_resource_ref BufferResource::pinned_mr() { | ||
| rmm::host_device_async_resource_ref BufferResource::pinned_mr() { | ||
| RAPIDSMPF_EXPECTS( | ||
| pinned_mr_, "no pinned memory resource is available", std::invalid_argument | ||
| ); | ||
|
|
@@ -196,14 +196,37 @@ std::unique_ptr<Buffer> BufferResource::allocate( | |
| } | ||
|
|
||
| std::unique_ptr<Buffer> BufferResource::move( | ||
| std::unique_ptr<rmm::device_buffer> data, rmm::cuda_stream_view stream | ||
| std::unique_ptr<rmm::device_buffer> data, | ||
| rmm::cuda_stream_view stream, | ||
| MemoryType mem_type | ||
| ) { | ||
| auto upstream = data->stream(); | ||
| if (upstream.value() != stream.value()) { | ||
| cuda_stream_join(stream, upstream); | ||
| data->set_stream(stream); | ||
| } | ||
| return std::unique_ptr<Buffer>(new Buffer(std::move(data), MemoryType::DEVICE)); | ||
|
|
||
| if (mem_type == MemoryType::DEVICE) { | ||
| return std::unique_ptr<Buffer>(new Buffer(std::move(data), MemoryType::DEVICE)); | ||
| } else if (mem_type == MemoryType::PINNED_HOST) { | ||
| RAPIDSMPF_EXPECTS( | ||
| pinned_mr_ != PinnedMemoryResource::Disabled, | ||
| "pinned memory resource is not available", | ||
| std::invalid_argument | ||
| ); | ||
|
|
||
| auto pinned_host_buffer = std::make_unique<HostBuffer>( | ||
| HostBuffer::from_rmm_device_buffer(std::move(data), stream, pinned_mr()) | ||
| ); | ||
| return std::unique_ptr<Buffer>( | ||
| new Buffer(std::move(pinned_host_buffer), stream, MemoryType::PINNED_HOST) | ||
| ); | ||
|
Comment on lines
+221
to
+226
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to think about this going forward. I don't really like the split in the type system between the pinned host buffer and the rmm::device_buffer storage. Remind me why we need the pinned host buffer to be stored as a HostBuffer inside the Buffer object?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This was required, because say we pass a pinned MR to a cudf operation, all allocations made by cudf are returned as rmm::device_buffer. So, we need a way to move it into a Host buffer.
PinnedBuffer and HostBuffer were merged together because both impls were very similar. So, now both pinned and host data ptr is passed on to the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @wence- this comment line it out-of-place now, because I changed the impl |
||
| } else { | ||
| RAPIDSMPF_FAIL( | ||
| "Invalid memory type: " + std::string(to_string(mem_type)), | ||
| std::invalid_argument | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| std::unique_ptr<Buffer> BufferResource::move( | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.