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

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

Conversation

ahendriksen
Copy link
Contributor

This PR isolates the logger and memory pool. These are likely to transitively include spdlog, which slows down compile times. After rapidsai/rmm#1241 is merged, this will yield a substantial improvement in compile times for smaller translation units.

It is a follow up to PR #1440.

Instead of having the specializations in sub-directories, the
raft_runtime source files now mimic the include/ directory hierarchy.
Used .data() instead of .data_handle()
These types are not used in the ext header, but are useful to have.
Under multiple combinations of RAFT_EXPLICIT_INSTANTIATE_ONLY and RAFT_COMPILED
The compute_similarity and interleaved_scan kernel are quite expensive
to compile. Splitting the headers in this commit.
These two include files are likely to transitively include spdlog, which
increases compilation times.
@ahendriksen ahendriksen requested review from a team as code owners April 20, 2023 18:53
@ahendriksen
Copy link
Contributor Author

ahendriksen commented Apr 20, 2023

Review tip: this PR is a follow up to PR #1440. Only the commit 6109c12 is part of this PR. That should reduce the diff considerably.

@ahendriksen ahendriksen added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Build Time Improvement labels Apr 21, 2023
@ahendriksen ahendriksen self-assigned this Apr 21, 2023
@ahendriksen
Copy link
Contributor Author

Note the difference in CI time for this PR compared to the first on in the series:

Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Reviewed the last commit, it looks good to me.

cpp/include/raft/util/memory_pool-inl.hpp Show resolved Hide resolved
@ahendriksen
Copy link
Contributor Author

This is a reminder to myself that this branch does not include commit b647aef that was added to PR #1440 .

Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

LGTM, pending changes in #1437. I verified the logger changes.

cpp/include/raft/core/logger-inl.hpp Outdated Show resolved Hide resolved
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

Sorry, @ahendriksen, I looked a little more closely at the macros and I'd like to consolidate these so we don't end up with too much duplication for these core bits. Otherwise, I think this looks great.

cpp/include/raft/util/inline.hpp Outdated Show resolved Hide resolved
@cjnolet cjnolet closed this Apr 28, 2023
rapids-bot bot pushed a commit that referenced this pull request Apr 28, 2023
This is a rebase of all the commits in PRs:
- #1437 
- #1438 
- #1439 
- #1440 
- #1441 

The original PRs have not been rebased to preserve review comments. This PR is up to date with branch 23.06.

Closes #1416

Authors:
  - Allard Hendriksen (https://github.com/ahendriksen)

Approvers:
  - Divye Gala (https://github.com/divyegala)
  - Corey J. Nolet (https://github.com/cjnolet)

URL: #1469
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Time Improvement CMake cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
Development

Successfully merging this pull request may close these issues.

4 participants