Move more implementations to precompiled shared library (part 2) - #1983
Conversation
| #if RMM_LOG_ACTIVE_LEVEL <= RMM_LOG_LEVEL_DEBUG | ||
| #ifdef CUDA_API_PER_THREAD_DEFAULT_STREAM | ||
| logger_.debug("----- RMM LOG [PTDS ENABLED] -----"); | ||
| #else | ||
| logger_.debug("----- RMM LOG [PTDS DISABLED] -----"); | ||
| #endif | ||
| #endif |
There was a problem hiding this comment.
Previously, because librmm was header-only, these macros could be controlled when the application using librmm (such as libcudf) was built.
However, moving these to a precompiled library means that the behavior could change, because it's controlled when librmm.so is built rather than the downstream application.
@vyasr @wence- @vuule Do you have thoughts on this? Part of me wants to remove the "PTDS enabled" macros and logger lines.
There was a problem hiding this comment.
Can this feature be queried at runtime?
There was a problem hiding this comment.
There's another call in the code that is more of a runtime check:
rmm/cpp/src/cuda_stream_view.cpp
Line 41 in fd5bb01
So I do agree with @ttnghia we may need a runtime check, some sort of static we are going to set at the beginning of the process.
There was a problem hiding this comment.
IMO, if we want to have a pre-compiled library that can be compiled once and work with multiple downstream applications that can have different settings for per-thread stream, this macro (CUDA_API_PER_THREAD_DEFAULT_STREAM) should better be removed completely and replaced by a static variable, which can be set by the downstream application.
There was a problem hiding this comment.
I would also like to remove the PTDS lines but I think that is wholly separate from most of the other considerations around what the loggers should do.
There are different ways to tell CUDA to use a stream per thread. Compiling with CUDA_API_PER_THREAD_DEFAULT_STREAM is the easiest one and what Spark does. You can also pass a stream value of cudaStreamPerThread (which rmm wraps as the cuda_stream_per_thread variable shown in that snippet linked by @abellina above) to each API at runtime. The runtime model is the more flexible, obviously, and it is what we plan to do in the Python layer (see NVIDIA/cudf#17626). The reason that Spark didn't start that way is because libcudf didn't fully support stream-ordering in all APIs until fairly recently.
Whether or not code should contain the CUDA_API_PER_THREAD_DEFAULT_STREAM macro is really a question of safety. If you still want someone to be able to clone the source and build a binary that behaves as expected, you should include the compile-time guard where appropriate. That is complementary to (not a replacement of) a runtime check.
| // cudaErrorInvalidValue is returned when non-managed memory is passed to | ||
| // cudaMemPrefetchAsync. We treat this as a no-op. |
There was a problem hiding this comment.
Is it possible if the result is still cudaErrorInvalidValue in the situations other than "non-managed memory is passed to"?
There was a problem hiding this comment.
Prefetching is safe to ignore on errors like this. We aren't aware of any other situations that trigger this error, but even if there were, it's okay to do nothing.
(Also this is only code movement, not a change in the behavior or comments from what we had in the header before.)
|
/merge |
Description
Continues from #1980.
Contributes to #1779.
Checklist