Move RMM_LOGGING_ASSERT into separate header #1241

ahendriksen · 2023-04-04T14:39:49Z

The inclusion of rmm/logger.hpp can increase compile times significantly. Before this PR, rmm/logger.hpp was included in rmm/error.hpp which by necessity was included almost everywhere. This PR moves RMM_LOGGING_ASSERT into its own header, thereby drastically cutting down on the headers which transitively include rmm/logger.hpp.

This PR will make it possible to reduce compile times of RAFT by ~10 seconds per translation unit.

Description

Related to issue #1222 and also PR #1232. Compared to #1232, this PR might make it able to also have fast builds without precompiling spdlog.

I include a table below showing which headers transitively include rmm/logger.hpp before and after PR (in debug and release builds). These are the rmm headers used by RAFT.

Header	Before	After
rmm/cuda_device.hpp	debug release
rmm/cuda_stream.hpp	debug release	debug
rmm/cuda_stream_pool.hpp	debug release	debug
rmm/cuda_stream_view.hpp	debug release
rmm/device_buffer.hpp	debug release
rmm/device_scalar.hpp	debug release
rmm/device_uvector.hpp	debug release
rmm/device_vector.hpp	debug release
rmm/exec_policy.hpp	debug release
rmm/logger.hpp	debug release	debug release
rmm/mr/device/aligned_resource_adaptor.hpp	debug release
rmm/mr/device/arena_memory_resource.hpp	debug release	debug release
rmm/mr/device/binning_memory_resource.hpp	debug release	debug release
rmm/mr/device/callback_memory_resource.hpp	debug release
rmm/mr/device/cuda_async_memory_resource.hpp	debug release
rmm/mr/device/cuda_async_view_memory_resource.hpp	debug release
rmm/mr/device/cuda_memory_resource.hpp	debug release
rmm/mr/device/device_memory_resource.hpp	debug release
rmm/mr/device/failure_callback_resource_adaptor.hpp	debug release
rmm/mr/device/fixed_size_memory_resource.hpp	debug release	debug release
rmm/mr/device/limiting_resource_adaptor.hpp	debug release
rmm/mr/device/logging_resource_adaptor.hpp	debug release	debug release
rmm/mr/device/managed_memory_resource.hpp	debug release
rmm/mr/device/owning_wrapper.hpp	debug release
rmm/mr/device/per_device_resource.hpp	debug release
rmm/mr/device/polymorphic_allocator.hpp	debug release
rmm/mr/device/pool_memory_resource.hpp	debug release	debug release
rmm/mr/device/statistics_resource_adaptor.hpp	debug release
rmm/mr/device/thread_safe_resource_adaptor.hpp	debug release
rmm/mr/device/thrust_allocator_adaptor.hpp	debug release
rmm/mr/device/tracking_resource_adaptor.hpp	debug release	debug release
rmm/mr/host/host_memory_resource.hpp
rmm/mr/host/new_delete_resource.hpp
rmm/mr/host/pinned_memory_resource.hpp	debug release

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

Avoiding the inclusion of rmm/logger.hpp can reduce compile times significantly.

ahendriksen · 2023-04-04T14:43:23Z

Since this is moving an internal macro from one detail header to another detail header, it should be considered a non-breaking change. In practice, it might break downstream projects that are expecting rmm/logger.hpp to be included. I had to fix up some files in the rmm tree for this reason. I am not sure how common it is for downstream projects to depend on rmm/logger.hpp to be included.

harrism · 2023-04-04T20:53:50Z

I include a table below showing which headers transitively include rmm/logger.hpp before and after PR (in debug and release builds). These are the rmm headers used by RAFT.

Does this PR only update the RMM headers used by RAFT? It would be better to update all headers, I think.

harrism · 2023-04-04T20:54:34Z

So does this replace #1232?

vyasr

I agree with Mark, we should update all headers in one PR. Afterwards, we should test that at least the core RAPIDS libraries continue to build successfully against this PR before merging.

If building cudf/cuml/cugraph is difficult for you to do locally, you can also do it fairly easily in CI by opening PRs to each repo that use the build artifacts from this PR rather than pulling the latest librmm nightly. There have been a few examples of this lately, including this commit in this raft PR. I would recommend following that pattern for at least cudf/cuml/cugraph to make sure that they compile successfully. Let me know if you need help with that.

harrism · 2023-04-05T02:20:39Z

Building locally should be easy with rapids-compose, if not devcontainers. Not sure if all repos have the devcontainers yet, but the cuspatial devcontainer can be used to build RMM, cuDF, and cuSpatial, for example.

ahendriksen · 2023-04-05T08:58:48Z

Does this PR only update the RMM headers used by RAFT? It would be better to update all headers, I think.

No, this PR updates all headers and makes sure all tests compile and run. I have included the table to show that the impact on one particular downstream library is meaningful. If you want to know for other headers what the impact is, feel free to edit and run this bash snippet:

export FILES=( rmm/cuda_stream.hpp rmm/cuda_stream_pool.hpp rmm/cuda_stream_view.hpp rmm/device_buffer.hpp rmm/device_scalar.hpp rmm/device_uvector.hpp rmm/device_vector.hpp rmm/exec_policy.hpp rmm/mr/device/device_memory_resource.hpp rmm/mr/device/limiting_resource_adaptor.hpp rmm/mr/device/managed_memory_resource.hpp rmm/mr/device/per_device_resource.hpp rmm/mr/device/pool_memory_resource.hpp rmm/mr/host/new_delete_resource.hpp rmm/mr/host/pinned_memory_resource.hpp rmm/mr/device/per_device_resource.hpp)

for f in ${FILES[@]}; do
    echo -n ${f};
    g++ -x c++  -H -I./include/ -I./build/_deps/spdlog-src/include -I./build/_deps/fmt-src/include -c <(echo "#include <${f}>") 2>&1| grep -q 'spdlog' && echo -n ' debug '
    g++ -x c++ -DNDEBUG -H -I./include/ -I./build/_deps/spdlog-src/include -I./build/_deps/fmt-src/include  -c <(echo "#include <${f}>") 2>&1| grep -q 'spdlog' && echo -n ' release '
    echo ""
done

So does this replace #1232?

That really depends on the direction that RMM sets for using logging. If RMM_LOGGING_ASSERT starts to be used in core include files, like cuda_stream_view, device_uvector, etc, then #1232 will be essential to preserve compile times for downstream libraries. On the other hand, if logging is used conservatively in relatively constrained places (like memory pool), then it is easy to work around in downstream libraries and PR #1232 might be more hassle than it is worth.

I agree with Mark, we should update all headers in one PR. Afterwards, we should test that at least the core RAPIDS libraries continue to build successfully against this PR before merging.

This PR updates all headers. I will open PRs on cudf/cuml/cugraph and report back.

If building cudf/cuml/cugraph is difficult for you to do locally, you can also do it fairly easily in CI by opening PRs to each repo that use the build artifacts from this PR rather than pulling the latest librmm nightly.

Thanks for the example! I have opened

As a general question, if moving a single macro in the detail headers requires rerunning CI in downstream libraries, would it make sense to automate it?

harrism · 2023-04-05T12:11:01Z

As a general question, if moving a single macro in the detail headers requires rerunning CI in downstream libraries, would it make sense to automate it?

Testing all of RAPIDS in every push to a RMM PR would be a bit of a burden both on our CI systems and costs, and on developer productivity.

However I believe this is automated in nightly tests.

ahendriksen · 2023-04-05T12:46:22Z

Status update:

the cuml cpp build + tests have succeeded.
the cugraph cpp build + tests have succeeded.
the cudf cpp build has failed because tests/utilities/identify_stream_usage.cpp uses strcmp but does not include string:

/cpp/tests/utilities/identify_stream_usage.cpp:162:35: error: 'strcmp' was not declared in this scope
2023-04-05T09:38:17.2091899Z   162 |     if (env_stream_error_mode && !strcmp(env_stream_error_mode, "print")) {
2023-04-05T09:38:17.2092304Z       |                                   ^~~~~~
2023-04-05T09:38:17.2092842Z $SRC_DIR/cpp/tests/utilities/identify_stream_usage.cpp:26:1: note: 'strcmp' is defined in header '<cstring>'; did you forget to '#include <cstring>'?

I had a typo in the python channel, so all python builds have failed.

How do you suggest I proceed?

ahendriksen · 2023-04-05T12:55:06Z

I have filed rapidsai/cudf#13066 to fix the missing include and pushed to the cuml, cudf, and cugraph PR a fix to the typo in the Python RMM channel.

ahendriksen · 2023-04-05T18:53:17Z

All test PRs are passing CI in cuml, cugraph, and cudf. I fixed two includes in cudf. One for algorithm and one for cstring.

I think we should be good to go. 👍

vyasr · 2023-04-08T00:03:40Z

As a general question, if moving a single macro in the detail headers requires rerunning CI in downstream libraries, would it make sense to automate it?

I mainly requested this because in the specific case of macros we have had many issues before where dependencies were using macros that were never intended to be public. In general, I don't think this level of testing is required.

That said, there are many cases where we do want certain changes to a library to trigger tests of the dependencies. I know that you see this all the time with raft->cuml/cugraph, and any rmm or rapids-cmake change will affect all of RAPIDS. We are interested in automating at least the dispatch process, but there are many tasks ahead of that in the queue. Running all of RAPIDS CI on every rmm PR is definitely overkill though.

ahendriksen · 2023-04-14T09:52:26Z

I mainly requested this because in the specific case of macros we have had many issues before where dependencies were using macros that were never intended to be public.

I agree that it is good to test the change on downstream libraries and I am happy that this was able to catch some missing includes in cudf.

That said, there are many cases where we do want certain changes to a library to trigger tests of the dependencies. [..] We are interested in automating at least the dispatch process, but there are many tasks ahead of that in the queue.

Great! Good to know that automating the dispatch process is in the queue.

I have tested this PR on a current branch of RAFT that I am working on. It reduced build times of libraft + tests by 10% (29 min to 26 min) and reduced build times per translation unit by 11s (median) and 8.5s (mean). There are still some big 'chunks' in the RAFT process that limit the overall impact. For incremental compilation, the impact can be quite large: most test files go from 20s to 10s compile time.

At the suggestion of @teju85, I have also checked with github code search for uses of RMM_LOGGING_ASSERT, but found none in other libraries (except for vendored copies of RMM in their directory structure).

harrism

I think this is a nice improvement.

ahendriksen · 2023-04-18T10:15:55Z

@vyasr : is there anything holding up this PR?

vyasr · 2023-04-19T18:38:45Z

@vyasr : is there anything holding up this PR?

Nope sorry I've just been on vacation and hadn't had a chance to review again. Looking now.

vyasr

For the record, could you update the table in the PR description? I'm not sure that it is up-to-date, there are a number of files that now include both logging_assert.hpp and logger.hpp. Was the goal of the table to only capture transitive includes, or to generally list all files where the state of logger.hpp inclusion has changed? Examples like fixed_size_memory_resource.hpp or pool_memory_resource.hpp added an include, but I suppose the end result is the same in terms of what symbols end up defined, so perhaps that's what you were aiming for?

Aside from that the change looks great, thanks!

ahendriksen · 2023-04-20T18:27:51Z

I have updated the table. It now contains all non-detail headers. Indeed it lists per header if it transitively includes spdlog. It was not the intention to list all files where the inclusion of logger.hpp has changed.

vyasr · 2023-04-20T22:08:39Z

Awesome thanks for that @ahendriksen, looks great!

vyasr · 2023-04-20T22:08:44Z

/merge

Move RMM_LOGGING_ASSERT into separate header

4ed3029

Avoiding the inclusion of rmm/logger.hpp can reduce compile times significantly.

ahendriksen requested a review from a team as a code owner April 4, 2023 14:39

ahendriksen requested review from vyasr and jrhemstad April 4, 2023 14:39

Merge branch 'branch-23.06' into enh-move-logging-assert

1fd0b84

github-actions bot added the cpp Pertains to C++ code label Apr 4, 2023

harrism added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Apr 4, 2023

vyasr requested changes Apr 4, 2023

View reviewed changes

ahendriksen mentioned this pull request Apr 5, 2023

[WIP] Test changes in RMM PR 1241 rapidsai/cuml#5331

Closed

This was referenced Apr 5, 2023

[WIP] Test changes in RMM PR 1241 rapidsai/cugraph#3419

Closed

[WIP] Test changes in RMM PR 1241 rapidsai/cudf#13064

Closed

ahendriksen mentioned this pull request Apr 14, 2023

Remove specializations and split expensive headers rapidsai/raft#1415

Closed

Merge branch 'branch-23.06' into enh-move-logging-assert

a63ceb5

harrism approved these changes Apr 18, 2023

View reviewed changes

Merge branch 'branch-23.06' into enh-move-logging-assert

401a492

Merge branch 'branch-23.06' into enh-move-logging-assert

34e32af

vyasr approved these changes Apr 19, 2023

View reviewed changes

ahendriksen mentioned this pull request Apr 20, 2023

[ENH] [5/5] Header structure: isolate logger and memory pool rapidsai/raft#1441

Closed

rapids-bot bot merged commit a7f5d77 into rapidsai:branch-23.06 Apr 20, 2023

ahendriksen mentioned this pull request May 2, 2023

Fix get_pool_memory_resource return type rapidsai/raft#1483

Closed

vyasr mentioned this pull request Jun 2, 2023

[BUG] Consider removing spdlog dependency for substantial compile time improvements #1222

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move RMM_LOGGING_ASSERT into separate header #1241

Move RMM_LOGGING_ASSERT into separate header #1241

ahendriksen commented Apr 4, 2023 •

edited

Loading

ahendriksen commented Apr 4, 2023

harrism commented Apr 4, 2023

harrism commented Apr 4, 2023

vyasr left a comment

harrism commented Apr 5, 2023

ahendriksen commented Apr 5, 2023 •

edited

Loading

harrism commented Apr 5, 2023

ahendriksen commented Apr 5, 2023

ahendriksen commented Apr 5, 2023 •

edited

Loading

ahendriksen commented Apr 5, 2023

vyasr commented Apr 8, 2023

ahendriksen commented Apr 14, 2023

harrism left a comment

ahendriksen commented Apr 18, 2023

vyasr commented Apr 19, 2023

vyasr left a comment

ahendriksen commented Apr 20, 2023 •

edited

Loading

vyasr commented Apr 20, 2023

vyasr commented Apr 20, 2023

Move RMM_LOGGING_ASSERT into separate header #1241

Move RMM_LOGGING_ASSERT into separate header #1241

Conversation

ahendriksen commented Apr 4, 2023 • edited Loading

Description

Checklist

ahendriksen commented Apr 4, 2023

harrism commented Apr 4, 2023

harrism commented Apr 4, 2023

vyasr left a comment

Choose a reason for hiding this comment

harrism commented Apr 5, 2023

ahendriksen commented Apr 5, 2023 • edited Loading

harrism commented Apr 5, 2023

ahendriksen commented Apr 5, 2023

ahendriksen commented Apr 5, 2023 • edited Loading

ahendriksen commented Apr 5, 2023

vyasr commented Apr 8, 2023

ahendriksen commented Apr 14, 2023

harrism left a comment

Choose a reason for hiding this comment

ahendriksen commented Apr 18, 2023

vyasr commented Apr 19, 2023

vyasr left a comment

Choose a reason for hiding this comment

ahendriksen commented Apr 20, 2023 • edited Loading

vyasr commented Apr 20, 2023

vyasr commented Apr 20, 2023

ahendriksen commented Apr 4, 2023 •

edited

Loading

ahendriksen commented Apr 5, 2023 •

edited

Loading

ahendriksen commented Apr 5, 2023 •

edited

Loading

ahendriksen commented Apr 20, 2023 •

edited

Loading