[FEA] libcudf stream parameters should default to cudaStreamPerThread when compiled for PTDS
#9614
Comments
jlowe
added
feature request
|
The alternative that wasn't mentioned and is what I would vastly prefer to see here is to expose streams in the public APIs of libcudf. #925 That way, a caller can specify whatever stream they like (including I'd much prefer that option to some non-standard swictheroo of defaulted function parameter values through compile time definitions. |
|
Yes, thanks @jrhemstad for fixing my omission. Exposing the stream parameter would be great and should remove the need for libcudf to be compiled with PTDS. I filed this mostly under the assumption #925 is still a long ways off -- is that no longer the case? |
|
This issue has been labeled |
|
This issue has been labeled |
|
Since nvcomp-2.3 is going to be released as only shared libraries. The solution in #9534, compiling nvcomp with |
|
Filed #10864 for solution. |
…fault_stream_value (#11082) Closes #10864. Also closes #9614. This PR is a follow-up to #10877. It replaces all of the remaining instances of `rmm::cuda_stream_default` with `cudf::default_stream_value`. There are a lot of replacements and addition of includes, along with some reformatting due to clang-format, but like #10877, there should be no noticeable functional change here. Authors: - Jim Brennan (https://github.com/jbrennan333) Approvers: - Robert (Bobby) Evans (https://github.com/revans2) - https://github.com/nvdbaranec - Nghia Truong (https://github.com/ttnghia) - Karthikeyan (https://github.com/karthikeyann) URL: #11082
Is your feature request related to a problem? Please describe.
#9534 occurred because libcudf was using a default-value argument as the stream to use (i.e.: the stream argument in the
detailAPIs that is currently hidden from the public APIs) and that stream was stream 0 (the default stream). Because libcudf was compiled for PTDS, it properly interpreted 0 ascudaStreamPerThreadbut this concept was not conveyed to other libraries called by libcudf. When 0 was passed to these other libraries, it ended up using the default stream instead ofcudaStreamPerThread. Ideally libcudf should pass the same stream it is using to external libraries that take a stream parameter. Then for libraries that are completely driven by stream parameters, those libraries don't have to be compiled with PTDS when libcudf is compiled with PTDS to avoid surprising performance pitfalls.Describe the solution you'd like
The default value of stream arguments in libcudf should be
cudaStreamPerThreadwhen libcudf is being compiled for PTDS. This will cause libcudf to pass the same stream it is using to external libraries that take a stream This idea was proposed by @jrhemstad in an offline discussion of solutions to #9534.Describe alternatives you've considered
A workaround is to compile all external libraries libcudf is using with PTDS when libcudf is compiled for PTDS, but this seems unnecessary for libraries like nvcomp which never uses the default stream unless directed by the application code to do so by passing the legacy default stream value as the stream to use.
Another option is to track down every place in libcudf where we pass a stream parameter to an external library and then pass
cudaStreamPerThreadif the stream is stream 0 and libcudf is compiled for PTDS. This seems more fragile in practice, as we have to manually locate all of these places, fix them (which may be tricky if the stream is passed into that code as a parameter), and remembering to fixup new external call sites going forward.The text was updated successfully, but these errors were encountered: