Use cudf::pack with pinned mr in TableChunk::copy#966
Use cudf::pack with pinned mr in TableChunk::copy#966nirandaperera wants to merge 11 commits intorapidsai:mainfrom
cudf::pack with pinned mr in TableChunk::copy#966Conversation
Signed-off-by: niranda perera <niranda.perera@gmail.com>
nirandaperera
added
improvement
|
This PR depends on #967 because it now requires precise packed sizes for pinned memory. |
pentschev
left a comment
pentschev
left a comment
There was a problem hiding this comment.
Looks good overall, left some requests for typo fixes and phrasing improvements, and a nit.
Co-authored-by: Peter Andreas Entschev <peter@entschev.com>
…pidsmpf into table_chunk_pinned_copy
Signed-off-by: niranda perera <niranda.perera@gmail.com>
| 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) | ||
| ); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I don't really like the split in the type system between the pinned host buffer and the rmm::device_buffer storage.
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.
Remind me why we need the pinned host buffer to be stored as a HostBuffer inside the Buffer object?
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 HostBuffer.
TBH I think we may be able to merge all buffer classes in to a single impl, with the rmm MR changes. Let's see.
Signed-off-by: niranda perera <niranda.perera@gmail.com>
3 participants
In a previous analysis we found that using
cudf::packwith a pinned mem resource performs similar to,Since (1) use unreserved device memory,
TableChunk::copyis prone to OOM. This PR changes pinned memory to usecudf::packdirectly.Depends on #967