[MINOR] Adding a RAPIDSMPF_DETAIL mode
#612
Conversation
Signed-off-by: niranda perera <[email protected]>
nirandaperera
added
improvement
Signed-off-by: niranda perera <[email protected]>
cpp/CMakeLists.txt
Outdated
| # Set RAPIDSMPF_DEBUG default based on build type | ||
| if(CMAKE_BUILD_TYPE STREQUAL "Debug") | ||
| set(RAPIDSMPF_DEBUG_DEFAULT ON) | ||
| else() | ||
| set(RAPIDSMPF_DEBUG_DEFAULT OFF) | ||
| endif() | ||
|
|
There was a problem hiding this comment.
I would prefer that the default is always off, even when CMake build type is debug, thus requiring you to specify it manually via --cmake-args, for example. The reason for that is we are more likely to build in debug mode to do general debugging than to require the extended NVTX annotations.
Furthermore, I think it makes more sense to have a different name, debug invokes a meaning of trying to resolve a problem, whereas I think this is more about performance validation/investigation. How do you feel about RAPIDSMPF_NVTX_DETAIL?
There was a problem hiding this comment.
A DEBUG mode macro came up in several discussions. So, I would like this to be used for not just nvtx annotations.
There was a problem hiding this comment.
RAPIDSMPF_VERBOSE or simply RAPIDSMPF_DETAIL?
There was a problem hiding this comment.
I think DEBUG is a more specific term IMO, I'd prefer RAPIDSMPF_DETAIL. In any case, I think we should only enable it when explicitly specified, and not just depend on CMAKE_BUILD_TYPE=Debug to enable it by default.
There was a problem hiding this comment.
Agree, let's not use DEBUG.
But DETAIL might not be ideal either. It could be confused with our detail namespace and with macros like RAPIDSMPF_CONCAT_DETAIL_
cpp/src/shuffler/shuffler.cpp
Outdated
| #if RAPIDSMPF_DEBUG | ||
| RAPIDSMPF_NVTX_SCOPED_RANGE("Shuffler.Progress", p_iters++); | ||
| #endif |
There was a problem hiding this comment.
Can we embed the RAPIDSMPF_DEBUG check inside RAPIDSMPF_NVTX_SCOPED_RANGE()?
There was a problem hiding this comment.
Doing that means we disable it completely unless built with that flag. Most software, cuDF inclusive, AFAIK, doesn't do that, NVTX symbols are always present. I think the primary goal of this PR is not disabling NVTX entirely, but rather the very verbose/fine-grained NVTX annotations.
There was a problem hiding this comment.
Then call it RAPIDSMPF_NVTX_SCOPED_RANGE_VERBOSE ?
There was a problem hiding this comment.
Yes, that option I like, +1 to that.
Signed-off-by: niranda perera <[email protected]>
nirandaperera
changed the title
DEBUG modeRAPIDSMPF_DETAIL mode
Signed-off-by: niranda perera <[email protected]>
KyleFromNVIDIA
left a comment
KyleFromNVIDIA
left a comment
There was a problem hiding this comment.
Approved CMake changes
Co-authored-by: Kyle Edwards <[email protected]>
| RAPIDSMPF_NVTX_SCOPED_RANGE("meta_recv"); | ||
| RAPIDSMPF_NVTX_SCOPED_RANGE_VERBOSE("meta_recv"); | ||
| #if RAPIDSMPF_VERBOSE_INFO | ||
| int i = 0; |
There was a problem hiding this comment.
Nit: now that this needs guards, rename i to something a bit more descriptive.
| #if RAPIDSMPF_VERBOSE_INFO | ||
| RAPIDSMPF_NVTX_MARKER("meta_recv_iters", i); | ||
| #endif |
There was a problem hiding this comment.
Or better than renaming i, how about adding a RAPIDSMPF_NVTX_MARKER_VERBOSE, then always defining i and let the RAPIDSMPF_NVTX_MARKER_VERBOSE determine whether to use i or not?
4 participants
This PR adds a detail definition to the build.
RAPIDSMPF_DETAILdef is available in c++.