[BUG] Data source host reads have a race because they are not stream-ordered
- Dominant language
- C++
- Stars
- 9.8k
- Forks
- 1.1k
- Avg merge
- 3d 6m
- Merged PRs (30d)
- 278
Description
The [`device_buffer_source` class](https://github.com/rapidsai/cudf/blob/827c1e6e88c2e6d044e31db72af19a0cc6baa0b0/cpp/src/io/utilities/datasource.cpp#L195) exposes the standard `host_read` interface. Reads in this case [use the global stream pool](https://github.com/rapidsai/cudf/blob/827c1e6e88c2e6d044e31db72af19a0cc6baa0b0/cpp/src/io/utilities/datasource.cpp#L204). However, since the data source is a device span, this is a potential race because the operation populating the buffer may have been queued up on a different stream, and there is no way for that information to be propagated to this call.
We have a few options here:
1. `host_read` could be changed to accept a stream and we could pass the user-provided stream all the way down. That would work, but might be confusing in cases where the datasource's input is not coming from stream-ordered operations since then the ordering guarantees don't apply.
2. Events could be threaded through by recording them on the input stream at the beginning of functions like `read_avro` and then waiting on them in downstream operations. That might necessitate hoisting stream pool usage out of data sources, though, and you would still need to change `host_read` to accept a stream. The difference between this and (1) is that it would still allow multiplexing of datasource reads after the input stream has reached the event.
3. Reader calls that are going to use the stream pool could synchronize the stream up front, e.g. [here for avro](https://github.com/rapidsai/cudf/blob/827c1e6e88c2e6d044e31db72af19a0cc6baa0b0/cpp/src/io/avro/reader_impl.cu#L461). That is generally a bad idea for all the usual performance reasons that make us avoid stream syncs, though.
4. Require the user to synchronize the stream before calling the API. Just as bad if not worse than 3 (since it also puts more onus on the user).
Option 2 seems like the best of these, while option 1 would be OK as well but is an API break. I'm open to other solutions as well.
Contributor guide
Assessment
This issue has not been assessed yet.