Use CUDF_JNI_ENABLE_PROFILING to conditionally enable profiling support. #12221
Conversation
java/src/main/native/CMakeLists.txt
Outdated
| @@ -87,6 +88,10 @@ if(CUDF_USE_PER_THREAD_DEFAULT_STREAM) | |||
| add_compile_definitions(CUDA_API_PER_THREAD_DEFAULT_STREAM CUDF_USE_PER_THREAD_DEFAULT_STREAM) | |||
| endif() | |||
|
|
|||
| if(CUDF_JNI_ENABLE_PROFILING) | |||
| target_compile_definitions(cudfjni PUBLIC CUDF_JNI_ENABLE_PROFILING) | |||
There was a problem hiding this comment.
@rapidsai/rapids-cmake-codeowners Can someone look at this CMake? Do I need a value here or something like "CUDF_JNI_ENABLE_PROFILING=1"? I don't feel super confident in this.
There was a problem hiding this comment.
I'm not a CMake expert, but this looks correct to me.
There was a problem hiding this comment.
Based on the build failure, I think this should be add_compile_definitions like the code just above.
There was a problem hiding this comment.
I am going to push a fix for this shortly. I think this CMake file has some issues. We should prefer target_compile_definitions to keep the scope limited, but add_library(cudfjni) occurs lower in the script. All target_compile_definitions must happen after the library is defined. This is also an issue for USE_NVTX, which I was using as an example. I am guessing that we have not recently tested the build with that option turned off.
There was a problem hiding this comment.
both CUDF_JNI_ENABLE_PROFILING and CUDF_JNI_ENABLE_PROFILING=1 are valid. It depends on if you want to define a name or a name with value.
#define CUDF_JNI_ENABLE_PROFILING
// versus
#define CUDF_JNI_ENABLE_PROFILING 1There was a problem hiding this comment.
But yes this needs to go after the construction of the target cudfjni
|
The JNI side of this looks great, I am not too sure on the CMake side of things either. |
bdice
added
improvement
java/src/main/native/CMakeLists.txt
Outdated
| @@ -87,6 +88,10 @@ if(CUDF_USE_PER_THREAD_DEFAULT_STREAM) | |||
| add_compile_definitions(CUDA_API_PER_THREAD_DEFAULT_STREAM CUDF_USE_PER_THREAD_DEFAULT_STREAM) | |||
| endif() | |||
|
|
|||
| if(CUDF_JNI_ENABLE_PROFILING) | |||
| target_compile_definitions(cudfjni PUBLIC CUDF_JNI_ENABLE_PROFILING) | |||
There was a problem hiding this comment.
I'm not a CMake expert, but this looks correct to me.
|
JNI build failed on:
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## branch-23.02 #12221 +/- ##
===============================================
Coverage ? 88.19%
===============================================
Files ? 137
Lines ? 22660
Branches ? 0
===============================================
Hits ? 19984
Misses ? 2676
Partials ? 0 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
@gpucibot merge |
Description
The conda cudatoolkit packages do not include
<cuda_profiler_api.h>. This PR makes CUDA profiling (introduced in #9543) optional in the JNI layer. By default, profiling will be enabled. In GitHub Actions (#12002), profiling will be disabled.Checklist