make data chunk reader return unique_ptr #9129
Conversation
…ne ownership of data
| @@ -65,19 +89,8 @@ class istream_data_chunk_reader : public data_chunk_reader { | |||
| } | |||
| } | |||
|
|
|||
| device_span<char> find_or_create_data(std::size_t size, rmm::cuda_stream_view stream) | |||
There was a problem hiding this comment.
Kind of hard to connect the RMM optimization and changes here. Was this a performance hack to avoid repeatedly allocating the chunks?
There was a problem hiding this comment.
the sync-and-steal behavior in rmm was stealing too often, preventing portions of work from overlapping. With the changes in rmm#851, we no longer have to work around that behavior.
There was a problem hiding this comment.
Updated the PR description to give a better overview of how the changes relate.
Codecov Report
@@ Coverage Diff @@
## branch-21.10 #9129 +/- ##
===============================================
Coverage ? 10.83%
===============================================
Files ? 114
Lines ? 19101
Branches ? 0
===============================================
Hits ? 2070
Misses ? 17031
Partials ? 0 Continue to review full report at Codecov.
|
vuule
added
non-breaking
|
@vuule it wasn't the case initially because there was no practical use case for the new classes, since ownership of the data was never returned to the caller. Now that there is a practical use case, the design is more obviously appropriate. |
cwharris
changed the title
|
@gpucibot merge |
Depends on rapidsai/rmm#851, for performance reasons.
There are two parts to this change. First, we remove a workaround for RMM's sync-and-steal behavior which was preventing some work from overlapping. This behavior is significantly improveed in rmm#851. The workaround involved allocating long-lived buffers and reusing them. With this change, we create device_uvectors on-the-fly and return them, which brings us to the second part of the change...
Because the data chunk reader owned the long-lived buffers, it was possible to return
device_spans from theget_next_chunkmethod. Now that thedevice_uvectors are created on the fly and returned, we need an interface that supports ownership of the data on an implementation basis. Different readers can return different implementations ofdevice_data_chunkvia aunique_ptr. Those implementations can be owners of data, or just views.This PR should merge only after rmm#851, else it will cause performance degradation in
multibyte_split(which is the only API to use this reader so far).