Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create logger wrapper around spdlog that can be easily reused in other libraries #1722

Merged
merged 122 commits into from
Nov 26, 2024
Merged
Show file tree
Hide file tree
Changes from 86 commits
Commits
Show all changes
122 commits
Select commit Hold shift + click to select a range
27b6cab
First version of new logger
vyasr Oct 26, 2024
aa92779
Make rmm backwards compat layers explicit
vyasr Oct 26, 2024
812b2bc
Move logger to its own directory
vyasr Oct 26, 2024
956fa3f
Set up basics of file generation
vyasr Oct 26, 2024
3a6d49b
Convert to a function
vyasr Oct 27, 2024
c9ec22c
Switch to using a target
vyasr Oct 27, 2024
2573e02
Make rmm compatibility and logging support controllable via CMake
vyasr Oct 27, 2024
9173108
Hide impl one level down
vyasr Oct 31, 2024
d89bb82
Make rapids_logger a CMake package
vyasr Nov 1, 2024
07dfcc5
Fix install rules
vyasr Nov 1, 2024
8194997
Switch arena to delegating constructor for simplicity
vyasr Nov 1, 2024
8589c53
Clean up some conditional compilation
vyasr Nov 1, 2024
f4f8350
Move some functions out of the impl
vyasr Nov 1, 2024
7da8d70
Get everything compiling and running except arena tests with the new …
vyasr Nov 1, 2024
54abd70
Get arena test passing
vyasr Nov 1, 2024
627d369
Support a different approach for custom class formatting
vyasr Nov 2, 2024
39fe1b2
Get rid of redundant fake_impl
vyasr Nov 2, 2024
4a6cc9d
Some cleanup and consolidation of ifdef blocks
vyasr Nov 2, 2024
91fab68
Compile an object library for the tests to avoid recompilation.
vyasr Nov 2, 2024
2f58cc5
Properly enable building without logging support
vyasr Nov 2, 2024
2c021ec
Clean up some todos
vyasr Nov 2, 2024
eb93bf2
Add documentation
vyasr Nov 2, 2024
7b449bc
Simplify options handling and remove unnecessary exporting rules
vyasr Nov 2, 2024
9f85459
Set default options expected for rmm
vyasr Nov 2, 2024
946573a
Add documentation and stop defaulting export set
vyasr Nov 2, 2024
19556a6
Add functions required by the Python API
vyasr Nov 2, 2024
4a0dd6b
Get the Python package building with the new logger
vyasr Nov 3, 2024
bec1a9f
Get Python package importable
vyasr Nov 4, 2024
0be4979
Get all tests passing
vyasr Nov 4, 2024
0d6b118
Consolidate enum conversion
vyasr Nov 4, 2024
adae837
Make logger work with cudf
vyasr Nov 6, 2024
b422af6
Go back to compatibility mode for now
vyasr Nov 6, 2024
f7ae2be
Fix benchmarks
vyasr Nov 7, 2024
6a3028c
Comment out bits in the Python build that we don't need yet
vyasr Nov 7, 2024
afc4dcf
Fix the flag in Python
vyasr Nov 7, 2024
4714322
Temporarily reinstate inline keyword to get rmm working.
vyasr Nov 7, 2024
08cb91b
Make Python target selection safe in all modes
vyasr Nov 7, 2024
aaf4b7c
Address first round of feedback
vyasr Nov 7, 2024
7c9134c
Prefix new CMake variables with RMM_ and ensure that SUPPORTS_LOGGING…
vyasr Nov 7, 2024
8fd9036
Get tests and benchmarks fully working when building without compatib…
vyasr Nov 7, 2024
e6ec9f4
Fix tests
vyasr Nov 7, 2024
62efe44
Fix handling of configurable inline
vyasr Nov 7, 2024
0c08a3f
Fix up Python builds and tests again for non-legacy case
vyasr Nov 7, 2024
ee33b25
Make arena printing work in backwards compatibility mode
vyasr Nov 8, 2024
28e7c21
Add set_pattern to logger
vyasr Nov 9, 2024
55b3926
Remove unnecessary level of indirection.
vyasr Nov 9, 2024
56ab836
Simplify logging_resource_adaptor constructor stack
vyasr Nov 9, 2024
a690620
Implement basic support for sinks to get logging_resource_adaptor wor…
vyasr Nov 10, 2024
19c82ef
Fix logger format strings
vyasr Nov 10, 2024
9faf1b3
Fix logger output to match old fmt prints
vyasr Nov 10, 2024
b9b14f5
Inline the extra log call, the extra lambda invocation shouldn't be t…
vyasr Nov 10, 2024
39b3da8
Update logger tests for when logging is not supported
vyasr Nov 10, 2024
f299fd2
Make compatible with backwards compatibility mode again
vyasr Nov 11, 2024
15cdb0c
Make notes on what changes will be needed to use hide all spdlog symb…
vyasr Nov 11, 2024
3e8e286
Make notes on tests expected to fail when logging is disabled
vyasr Nov 11, 2024
440d7c1
Always hide the visibility of the logger implementation bits.
vyasr Nov 12, 2024
704c8e2
Add all notes on how we want to use spdlog to fully hide symbols in t…
vyasr Nov 12, 2024
5d11768
Make sure the unique_ptr is not default instantiated
vyasr Nov 13, 2024
ebdd741
Merge remote-tracking branch 'upstream/branch-24.12' into feat/logger
vyasr Nov 14, 2024
b8f22ec
Merge branch 'branch-25.02' into feat/logger
vyasr Nov 15, 2024
1fdee97
Remove all compatibility logic
vyasr Nov 16, 2024
b372c69
Resort the file to reduce complexity for the reader
vyasr Nov 16, 2024
6f6603a
Remove now redundant bytes formatting struct
vyasr Nov 16, 2024
b102d62
Move implementation of formatted_log from rmm to rapids_logger
vyasr Nov 16, 2024
1b3add3
Inline formatted_log into log
vyasr Nov 16, 2024
6379a72
More consolidation and remove unnecessary init_logger impl
vyasr Nov 16, 2024
4ef279d
Remove use of rmm error-handling macro
vyasr Nov 16, 2024
eb6ef9f
Clean up some includes
vyasr Nov 16, 2024
00f5577
Expose the default log filename publicly
vyasr Nov 16, 2024
cd71f30
Enable dynamic linking to spdlog
vyasr Nov 17, 2024
073e179
Make logging macros project-specific
vyasr Nov 17, 2024
853b8a1
Add README
vyasr Nov 17, 2024
92d5e21
Define default logging level
vyasr Nov 17, 2024
1ca5e8d
Try removing the exclude-libs
vyasr Nov 17, 2024
7bd2086
Compile spdlog with fpic
vyasr Nov 17, 2024
7928515
Make sure spdlog is available for compiling tests that need it.
vyasr Nov 17, 2024
6d6a4e4
Fix test that needs spdlog
vyasr Nov 17, 2024
cc2c1f9
Remove redundant flag
vyasr Nov 17, 2024
0985006
Always set fpic
vyasr Nov 17, 2024
3ca0f10
Also set up linkage for PTDS tests
vyasr Nov 17, 2024
46e650b
Also compile the logger itself with fpic
vyasr Nov 17, 2024
b450cfb
Remove unnecessary inline.
vyasr Nov 17, 2024
dd906e0
Switch to using bundled fmt
vyasr Nov 17, 2024
aed8320
Also set interface_position_independent_code
vyasr Nov 17, 2024
f5830c7
Fix title name
vyasr Nov 18, 2024
7020b9b
Don't fail on symbol errors to see if CI passes
vyasr Nov 18, 2024
e068335
Don't use CMAKE_INSTALL_PREFIX
vyasr Nov 20, 2024
4520ccb
Add support for callbacks
vyasr Nov 20, 2024
0abd94c
Remove dynamic spdlog
vyasr Nov 22, 2024
fe19b2b
Remove support for building without logging support
vyasr Nov 22, 2024
2ffc827
Go back to checking symbols
vyasr Nov 22, 2024
2731a53
Switch back to header-only spdlog
vyasr Nov 22, 2024
0a7b337
Revert "Add support for callbacks"
vyasr Nov 22, 2024
4f6b98c
Revert "Go back to checking symbols"
vyasr Nov 22, 2024
02212f1
Make sure symbols are not exported (required on both compile and link…
vyasr Nov 23, 2024
4e7eba7
Put back symbol check, adapted for new functions
vyasr Nov 23, 2024
2fcfc70
Remove unnecessary link option
vyasr Nov 23, 2024
49368cf
Add notes
vyasr Nov 25, 2024
46786c6
Full PImpl for the logger
vyasr Nov 25, 2024
8f6d03c
Remove now superfluous function supports_logging
vyasr Nov 25, 2024
f88e65a
Hide more in anonymous namespace
vyasr Nov 25, 2024
82443bd
Create a sink_impl
vyasr Nov 26, 2024
483bdfc
Implement a sink_vector as the primary intermediary for sinks.
vyasr Nov 26, 2024
316b1f9
Comment
vyasr Nov 26, 2024
a6bc3a7
Convert the underlying logger to a stack-allocated one.
vyasr Nov 26, 2024
3cf1074
Remove one unnecessary friend
vyasr Nov 26, 2024
155c934
Remove one more friend
vyasr Nov 26, 2024
b175d68
Make things consistent so that the sink_vector is the only friend tha…
vyasr Nov 26, 2024
9619e51
Add documentation
vyasr Nov 26, 2024
fa3b95e
Bugfix
vyasr Nov 26, 2024
e1fc36a
Inline init_logger
vyasr Nov 26, 2024
c5f56fd
Fix some comments
vyasr Nov 26, 2024
15697c7
Fixes for tracking_mr test
vyasr Nov 26, 2024
d12bb93
Fix sinks definition
vyasr Nov 26, 2024
2d84f2d
Remove support for mixing in spdlog sinks and update tests accordingly.
vyasr Nov 26, 2024
e7b2d92
Remove lingering spdlog include
vyasr Nov 26, 2024
b6af231
Update symbol checking script, we should no longer leak any of these …
vyasr Nov 26, 2024
82f925a
Remove visibility macro configurability from public header since we a…
vyasr Nov 26, 2024
6dfc38a
Try using macros
vyasr Nov 26, 2024
cae6784
Make sure that internally needed symbols are exported
vyasr Nov 26, 2024
6bbc174
Fix sink_ptr in docs
vyasr Nov 26, 2024
a21dedc
Add comments
vyasr Nov 26, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ message(STATUS "RMM: RMM_LOGGING_LEVEL = '${RMM_LOGGING_LEVEL}'")
# cudart can be linked statically or dynamically
option(CUDA_STATIC_RUNTIME "Statically link the CUDA runtime" OFF)

option(RMM_SUPPORTS_LOGGING "Enable logging support" ON)
option(RMM_SPDLOG_DYNAMIC "Use dynamic linking for spdlog" OFF)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows local builds to choose to use a dynamic spdlog and also use one that already exists on the system. If this is off, we currently always download for static linking under the assumption that we prioritize symbol hiding.


# ##################################################################################################
# * compiler options -------------------------------------------------------------------------------

Expand All @@ -73,7 +76,14 @@ rapids_find_package(
# add third party dependencies using CPM
rapids_cpm_init()

include(cmake/thirdparty/get_spdlog.cmake)
add_subdirectory(rapids_logger)
set(_logging_support_flag)
if(RMM_SUPPORTS_LOGGING)
set(_logging_support_flag "SUPPORTS_LOGGING")
endif()
rapids_make_logger(rmm EXPORT_SET rmm-exports VISIBILITY_MACRO
"__attribute__((visibility(\"default\")))" ${_logging_support_flag})

include(cmake/thirdparty/get_cccl.cmake)
include(cmake/thirdparty/get_nvtx.cmake)

Expand All @@ -94,8 +104,8 @@ else()
target_link_libraries(rmm INTERFACE CUDA::cudart)
endif()

target_link_libraries(rmm INTERFACE rmm_logger)
target_link_libraries(rmm INTERFACE CCCL::CCCL)
target_link_libraries(rmm INTERFACE spdlog::spdlog_header_only)
target_link_libraries(rmm INTERFACE dl)
target_link_libraries(rmm INTERFACE nvtx3::nvtx3-cpp)
target_compile_features(rmm INTERFACE cxx_std_17 $<BUILD_INTERFACE:cuda_std_17>)
Expand Down
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -658,8 +658,8 @@ of more detailed logging. The default is `INFO`. Available levels are `TRACE`, `
The log relies on the [spdlog](https://github.com/gabime/spdlog.git) library.

Note that to see logging below the `INFO` level, the application must also set the logging level at
run time. C++ applications must must call `rmm::logger().set_level()`, for example to enable all
levels of logging down to `TRACE`, call `rmm::logger().set_level(spdlog::level::trace)` (and compile
run time. C++ applications must must call `rmm::default_logger().set_level()`, for example to enable all
levels of logging down to `TRACE`, call `rmm::default_logger().set_level(spdlog::level::trace)` (and compile
librmm with `-DRMM_LOGGING_LEVEL=TRACE`). Python applications must call `rmm.set_logging_level()`,
for example to enable all levels of logging down to `TRACE`, call `rmm.set_logging_level("trace")`
(and compile the RMM Python module with `-DRMM_LOGGING_LEVEL=TRACE`).
Expand Down
16 changes: 14 additions & 2 deletions benchmarks/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,21 @@ function(ConfigureBench BENCH_NAME)
RUNTIME_OUTPUT_DIRECTORY "$<BUILD_INTERFACE:${RMM_BINARY_DIR}/gbenchmarks>"
CUDA_ARCHITECTURES "${CMAKE_CUDA_ARCHITECTURES}"
INSTALL_RPATH "\$ORIGIN/../../../lib")
target_link_libraries(${BENCH_NAME} benchmark::benchmark pthread rmm)
target_link_libraries(${BENCH_NAME} PRIVATE benchmark::benchmark pthread rmm)
target_compile_definitions(${BENCH_NAME}
PUBLIC "SPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_${RMM_LOGGING_LEVEL}")
PUBLIC "RMM_LOG_ACTIVE_LEVEL=RMM_LOG_LEVEL_${RMM_LOGGING_LEVEL}")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since each logger produced by rapids-logger gets its own set of logger macros controls, rmm's logging level is now decoupled from spdlog's.


if(PER_THREAD_DEFAULT_STREAM)
target_compile_definitions(${BENCH_NAME} PUBLIC CUDA_API_PER_THREAD_DEFAULT_STREAM)
endif()

target_compile_options(${BENCH_NAME} PUBLIC $<$<COMPILE_LANG_AND_ID:CXX,GNU,Clang>:-Wall -Werror
-Wno-unknown-pragmas>)

if(TARGET rmm_bench_logger)
target_link_libraries(${BENCH_NAME} PRIVATE rmm_bench_logger)
endif()

if(DISABLE_DEPRECATION_WARNING)
target_compile_options(
${BENCH_NAME} PUBLIC $<$<COMPILE_LANGUAGE:CUDA>:-Xcompiler=-Wno-deprecated-declarations>)
Expand All @@ -58,6 +63,13 @@ function(ConfigureBench BENCH_NAME)
EXCLUDE_FROM_ALL)
endfunction(ConfigureBench)

# Create an object library for the logger so that we don't have to recompile it. This is not
# necessary if logging is unsupported since then the header-only implementation is sufficient.
if(RMM_SUPPORTS_LOGGING)
add_library(rmm_bench_logger OBJECT)
target_link_libraries(rmm_bench_logger PRIVATE rmm_logger_impl)
endif()

# random allocations benchmark
ConfigureBench(RANDOM_ALLOCATIONS_BENCH random_allocations/random_allocations.cpp)

Expand Down
4 changes: 2 additions & 2 deletions benchmarks/replay/replay.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@
#include <benchmarks/utilities/cxxopts.hpp>
#include <benchmarks/utilities/log_parser.hpp>
#include <benchmarks/utilities/simulated_memory_resource.hpp>
#include <spdlog/common.h>

#include <atomic>
#include <chrono>
#include <condition_variable>
#include <iterator>
#include <memory>
#include <numeric>
Expand Down Expand Up @@ -403,7 +403,7 @@ int main(int argc, char** argv)
auto const num_threads = per_thread_events.size();

// Uncomment to enable / change default log level
// rmm::detail::logger().set_level(spdlog::level::trace);
// rmm::logger().set_level(rmm::level_enum::trace);

if (args.count("resource") > 0) {
std::string mr_name = args["resource"].as<std::string>();
Expand Down
3 changes: 2 additions & 1 deletion ci/check_symbols.sh
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ See https://cmake.org/cmake/help/latest/prop_tgt/LANG_VISIBILITY_PRESET.html and

echo ""
echo "${err_msg}"
exit 1
# TODO: Put this back once we decide what to check.
#exit 1
vyasr marked this conversation as resolved.
Show resolved Hide resolved
}

WHEEL_EXPORT_DIR="$(mktemp -d)"
Expand Down
36 changes: 30 additions & 6 deletions cmake/thirdparty/get_spdlog.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,39 @@
# the License.
# =============================================================================

# Use CPM to find or clone speedlog
# Use CPM to find or clone speedlog TODO: The logic here should be upstreamed into rapids-logger and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Use CPM to find or clone speedlog TODO: The logic here should be upstreamed into rapids-logger and
# Use CPM to find or clone spdlog TODO: The logic here should be upstreamed into rapids-logger and

# probably removed from rapids-cmake. Note that it is not possible to build with two different modes
# for different projects in the same build environment, so this functionality must be kept
# independent of the rapids_make_logger function. Alternatively, we could update the cpm calls for
# spdlog to not promote targets to global scope and instead allow each project to find and configure
# spdlog independently.
vyasr marked this conversation as resolved.
Show resolved Hide resolved
function(find_and_configure_spdlog)

include(${rapids-cmake-dir}/cpm/spdlog.cmake)
rapids_cpm_spdlog(
FMT_OPTION "EXTERNAL_FMT_HO"
INSTALL_EXPORT_SET rmm-exports
BUILD_EXPORT_SET rmm-exports)

if(RMM_SPDLOG_DYNAMIC)
rapids_cpm_spdlog(
FMT_OPTION "BUNDLED"
INSTALL_EXPORT_SET rmm-exports
BUILD_EXPORT_SET rmm-exports OPTIONS "SPDLOG_BUILD_SHARED ON" "BUILD_SHARED_LIBS ON")
else()
# For static spdlog usage assume that we want to hide as many symbols as possible, so do not use
# pre-built libraries. It's quick enough to build that there is no real benefit to supporting
vyasr marked this conversation as resolved.
Show resolved Hide resolved
# the alternative.
set(CPM_DOWNLOAD_spdlog ON)
rapids_cpm_spdlog(
# TODO: Is this safe to set up for all projects? Do we have to worry about the fmt patch
vyasr marked this conversation as resolved.
Show resolved Hide resolved
# currently in rapids-cmake? We should never be compiling files using spdlog under nvcc
# anymore.
FMT_OPTION "BUNDLED"
INSTALL_EXPORT_SET rmm-exports
BUILD_EXPORT_SET rmm-exports OPTIONS "SPDLOG_BUILD_SHARED OFF" "BUILD_SHARED_LIBS OFF"
EXCLUDE_FROM_ALL)
# Can't make both cmake-format and cmake-lint happy here.
# cmake-format: off
set_target_properties(spdlog PROPERTIES CXX_VISIBILITY_PRESET hidden VISIBILITY_INLINES_HIDDEN ON)
# cmake-format: on
endif()
set_target_properties(spdlog PROPERTIES POSITION_INDEPENDENT_CODE ON)
endfunction()

find_and_configure_spdlog()
48 changes: 0 additions & 48 deletions include/rmm/detail/format.hpp
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The functions removed here are now embedded directly in the logger's log method.

Original file line number Diff line number Diff line change
Expand Up @@ -20,60 +20,12 @@

#include <array>
#include <cstdio>
#include <ios>
#include <iostream>
#include <memory>
#include <sstream>
#include <stdexcept>
#include <string>

namespace RMM_NAMESPACE {
namespace detail {

/**
* @brief Format a message string with printf-style formatting
*
* This function performs printf-style formatting to avoid the need for fmt
* or spdlog's own templated APIs (which would require exposing spdlog
* symbols publicly) and returns the formatted message as a `std::string`.
*
* @param format The format string
* @param args The format arguments
* @return The formatted message
* @throw rmm::logic_error if an error occurs during formatting
*/
template <typename... Args>
std::string formatted_log(std::string const& format, Args&&... args)
{
auto convert_to_c_string = [](auto&& arg) -> decltype(auto) {
using ArgType = std::decay_t<decltype(arg)>;
if constexpr (std::is_same_v<ArgType, std::string>) {
return arg.c_str();
} else {
return std::forward<decltype(arg)>(arg);
}
};

// NOLINTBEGIN(cppcoreguidelines-pro-type-vararg)
auto retsize =
std::snprintf(nullptr, 0, format.c_str(), convert_to_c_string(std::forward<Args>(args))...);
RMM_EXPECTS(retsize >= 0, "Error during formatting.");
if (retsize == 0) { return {}; }
auto size = static_cast<std::size_t>(retsize) + 1; // for null terminator
// NOLINTNEXTLINE(modernize-avoid-c-arrays, cppcoreguidelines-avoid-c-arrays)
std::unique_ptr<char[]> buf(new char[size]);
std::snprintf(buf.get(), size, format.c_str(), convert_to_c_string(std::forward<Args>(args))...);
// NOLINTEND(cppcoreguidelines-pro-type-vararg)
return {buf.get(), buf.get() + size - 1}; // drop '\0'
}

// specialization for no arguments
template <>
inline std::string formatted_log(std::string const& format)
{
return format;
}

// Stringify a size in bytes to a human-readable value
inline std::string format_bytes(std::size_t value)
{
Expand Down
2 changes: 1 addition & 1 deletion include/rmm/detail/logging_assert.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
*/
#ifdef NDEBUG
#define RMM_LOGGING_ASSERT(_expr) (void)0
#elif SPDLOG_ACTIVE_LEVEL < SPDLOG_LEVEL_OFF
#elif RMM_LOG_ACTIVE_LEVEL < RMM_LOG_LEVEL_OFF
#define RMM_LOGGING_ASSERT(_expr) \
do { \
bool const success = (_expr); \
Expand Down
117 changes: 0 additions & 117 deletions include/rmm/logger.hpp

This file was deleted.

Loading
Loading