Convert part of RMM to a precompiled library #1896

bdice · 2025-04-24T14:14:08Z

Description

This PR is a starting point for #1779.

It converts RMM to a precompiled shared library, and moves some implementations into src/ files.

Checklist

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

copy-pr-bot · 2025-04-24T14:14:11Z

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

bdice · 2025-04-24T14:14:24Z

/ok to test

wence- · 2025-04-24T15:06:35Z

cpp/include/rmm/aligned.hpp

-{
-  return is_pow2(alignment);
-}
+[[nodiscard]] RMM_EXPORT bool is_supported_alignment(std::size_t alignment) noexcept;


FWIW, I believe RMM_NAMESPACE already contains the RMM_EXPORT macro, but it's not bad to be explicit. If we wanted to, we could then go back to just saying namespace rmm in this file.

We should, however, probably add RMM_EXPORT to the static symbols as well in that case.

Thanks for the explanation! I was unsure about this. I will think some more about what solution seems the most natural. Related to the other comment below, I would like it if we could use either RMM_NAMESPACE or namespace rmm in both headers and implementation files -- but that is tricky with the need to export things.

What do you think of this proposal:

get rid of RMM_NAMESPACE

don't put RMM_EXPORT on every symbol, just the namespaces as we currently do

use namespace RMM_EXPORT rmm in the headers and namespace rmm in the implementation files

On the topic of namespacing, I think there will also be a lot of detail functions that we can move from headers to implementation files in anonymous namespaces. (Currently we can't use anonymous namespaces because it's header-only and that's not good practice.)

wrt the proposal, that sounds reasonable. Ideally we move to a situation where everything in the detail namespace is __attribute__((visibility("hidden"))), similar to cudf last year:

Hide visibility of non public symbols cudf#15982

Move kernel vis over to CUDF_HIDDEN cudf#16165

Remove CUDA whole compilation ODR violations cudf#16603

I suppose your anonymous namespace suggestion would have the same effect.

Yes, for sure. That will likely be for a later pass. I haven't touched any "detail" headers yet.

wence- · 2025-04-24T15:07:26Z

cpp/src/aligned.cpp

+#include <cstddef>
+#include <cstdint>
+
+namespace RMM_NAMESPACE {


I believe here we should be using namespace rmm since it is the header file which defines our public exported API.

bdice · 2025-04-25T04:37:21Z

/ok to test

bdice · 2025-04-25T15:19:11Z

I'm going to cut the scope here for the first pass. I'd like to be sure that things work downstream before expanding the precompiled code further.

bdice · 2025-04-28T17:00:37Z

This is now ready for review. I am going to do some downstream testing with cuDF before I remove the DO NOT MERGE label.

cpp/include/rmm/cuda_device.hpp

cpp/CMakeLists.txt

Co-authored-by: Robert Maynard <[email protected]>

vyasr

Some minor notes, but generally LGTM.

cpp/CMakeLists.txt

vyasr · 2025-04-28T19:12:15Z

cpp/CMakeLists.txt

+             CXX_STANDARD_REQUIRED ON
+             CXX_VISIBILITY_PRESET hidden
+             POSITION_INDEPENDENT_CODE ON
+             INTERFACE_POSITION_INDEPENDENT_CODE ON)


We may be able to drop this eventually depending on how much of rmm stops being header-only. Ideally we wouldn't be proscribing this for consumers I think, @robertmaynard any thoughts?

Ideally once RMM is a proper library we can drop INTERFACE_POSITION_INDEPENDENT_CODE but currently having it won't hurt downstream projects

vyasr · 2025-04-28T19:13:26Z

cpp/include/rmm/aligned.hpp

 #include <cstdint>

-namespace RMM_NAMESPACE {
+namespace RMM_EXPORT rmm {


Is the idea to get rid of RMM_NAMESPACE altogether eventually?

Yes, see: #1896 (comment)

jrhemstad · 2025-04-28T20:18:26Z

What's the ultimate goal here? Convert everything that can be part of a precompiled binary into one?

Strictly speaking, the only thing necessary to be part of a DLL is the current device resource machinery.

vyasr · 2025-04-28T20:34:37Z

In my view the ultimate goal should be that if we are making rmm a shared library we should move anything that does not benefit from being header-only (for instance, non-template types that do not need to be inlined into performance-critical code paths) into the shared library. The current device resource machinery obviously needs to move for the uniqueness reasons, but moving other bits will also help us more easily make stability guarantees and reduce recompilation times across RAPIDS (right now a one-line change in the wrong place in rmm can bust sccache for the world, for example).

bdice · 2025-04-28T22:23:00Z

In my view the ultimate goal should be that if we are making rmm a shared library we should move anything that does not benefit from being header-only (for instance, non-template types that do not need to be inlined into performance-critical code paths) into the shared library. The current device resource machinery obviously needs to move for the uniqueness reasons, but moving other bits will also help us more easily make stability guarantees and reduce recompilation times across RAPIDS (right now a one-line change in the wrong place in rmm can bust sccache for the world, for example).

Yes -- all of the above here. Memory resources are the most important reason. Moving most code into a precompiled library also gives us stronger control over interfaces. We can shield implementation details behind anonymous namespaces and not need to expose them in detail (but "public") headers. We can start to have stability in our API/ABI that is more easily separable from implementation.

python/librmm/librmm/load.py

harrism

I'm going to leave this review at one question before I go on, because a lot of my opinions hinge on whether there's something I don't know about the answer to this question.

cpp/include/rmm/aligned.hpp

bdice · 2025-05-06T16:19:01Z

The next step needed for this is to add --exclude librmm.so to all the auditwheel invocations for downstream RAPIDS wheels using RMM (cudf, cugraph, ...).

bdice · 2025-05-07T20:00:08Z

I finished adding --exclude librmm.so to all the downstream libraries. I am doing final testing in rapidsai/cudf#18586 and then plan to merge this.

bdice · 2025-05-08T21:04:11Z

I concluded my testing in rapidsai/cudf#18586 through a combination of CI jobs and local CI reproductions (I hadn't set up wheel artifact downloads properly in CI but all seems fine locally when installing RMM wheel artifacts from this PR). I also dealt with a fair bit of friction from the new GitHub Artifacts work. rapidsai/gha-tools#172 will fix those points of friction, making it easier to do future testing.

I am planning to merge this over the weekend so that I can avoid the possibility of Friday/weekend breakage, and help deal with any problems it may cause on Monday.

bdice · 2025-05-11T11:37:31Z

/merge

After rapidsai#1896 the librmm wheel contains a compiled library, which means it is no longer accurate to tag the wheel as supported on any platform. We must tag it according to the suitable architecture tags to ensure that we do not get arm binaries on x86 and vice versa. We must also add a manylinux tag to indicate glibc compatibility. Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Kyle Edwards (https://github.com/KyleFromNVIDIA) URL: rapidsai#1913

Continues from #1896. Contributes to #1779. Authors: - Bradley Dice (https://github.com/bdice) Approvers: - Lawrence Mitchell (https://github.com/wence-) - Rong Ou (https://github.com/rongou) - Robert Maynard (https://github.com/robertmaynard) URL: #1980

Add RMM shared library, convert aligned.hpp functions.

0bbb038

github-project-automation bot added this to RMM Project Board Apr 24, 2025

github-actions bot added the CMake label Apr 24, 2025

bdice added feature request New feature or request breaking Breaking change labels Apr 24, 2025

wence- reviewed Apr 24, 2025

View reviewed changes

Convert cuda_device.hpp.

008a99b

bdice changed the title ~~Add RMM shared library, convert aligned.hpp functions.~~ Convert part of RMM to a precompiled library Apr 25, 2025

bdice added the DO NOT MERGE Hold off on merging; see PR for details label Apr 25, 2025

Convert cuda_stream_pool.hpp.

0637f84

bdice force-pushed the shared-library branch from 87c8b30 to 0262ab3 Compare April 25, 2025 14:33

Convert cuda_stream_view.hpp.

bfd63a5

bdice force-pushed the shared-library branch from 0262ab3 to bfd63a5 Compare April 25, 2025 14:39

bdice added 2 commits April 25, 2025 10:05

Fixes for builds/tests.

4113e98

Merge branch 'branch-25.06' into shared-library

ee37f7c

bdice marked this pull request as ready for review April 25, 2025 15:18

bdice requested review from a team as code owners April 25, 2025 15:18

bdice requested review from robertmaynard, vyasr and wence- April 25, 2025 15:18

github-actions bot added the Python Related to RMM Python API label Apr 25, 2025

bdice added 2 commits April 25, 2025 17:27

Update namespaces, migrate cuda_stream.hpp.

dd26598

Update build scripts

d22cd09

bdice requested a review from a team as a code owner April 27, 2025 19:14

robertmaynard requested changes Apr 28, 2025

View reviewed changes

cpp/include/rmm/cuda_device.hpp Outdated Show resolved Hide resolved

github-project-automation bot moved this to Review in RMM Project Board Apr 28, 2025

robertmaynard requested changes Apr 28, 2025

View reviewed changes

cpp/CMakeLists.txt Outdated Show resolved Hide resolved

cpp/CMakeLists.txt Outdated Show resolved Hide resolved

cpp/CMakeLists.txt Show resolved Hide resolved

bdice and others added 3 commits April 28, 2025 13:32

Remove RMM_EXPORT from each public API.

a29a2f9

Remove CUDA properties.

dbd6b85

Remove SHARED from rmm library declaration.

dd18602

Co-authored-by: Robert Maynard <[email protected]>

bdice requested a review from robertmaynard April 28, 2025 18:34

robertmaynard approved these changes Apr 28, 2025

View reviewed changes

vyasr approved these changes Apr 28, 2025

View reviewed changes

Matt711 approved these changes Apr 29, 2025

View reviewed changes

python/librmm/librmm/load.py Show resolved Hide resolved

Remove comment.

9278dbb

harrism reviewed Apr 29, 2025

View reviewed changes

cpp/include/rmm/aligned.hpp Show resolved Hide resolved

Merge branch 'branch-25.06' into shared-library

c2b1911

bdice self-assigned this May 7, 2025

bdice removed the DO NOT MERGE Hold off on merging; see PR for details label May 11, 2025

Merge branch 'branch-25.06' into shared-library

219e068

rapids-bot bot merged commit 8e19009 into rapidsai:branch-25.06 May 11, 2025
72 checks passed

github-project-automation bot moved this from Review to Done in RMM Project Board May 11, 2025

vyasr mentioned this pull request May 12, 2025

Use auditwheel to properly retag the wheel #1913

Merged

3 tasks

bdice mentioned this pull request Jul 9, 2025

Move more implementations to precompiled shared library #1980

Merged

3 tasks

Convert part of RMM to a precompiled library #1896

Convert part of RMM to a precompiled library #1896

Uh oh!

Conversation

bdice commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

copy-pr-bot bot commented Apr 24, 2025

Uh oh!

bdice commented Apr 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdice Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdice Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdice commented Apr 25, 2025

Uh oh!

bdice commented Apr 25, 2025

Uh oh!

bdice commented Apr 28, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vyasr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrhemstad commented Apr 28, 2025

Uh oh!

vyasr commented Apr 28, 2025

Uh oh!

bdice commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

harrism left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bdice commented May 6, 2025

Uh oh!

bdice commented May 7, 2025

Uh oh!

bdice commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdice commented May 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

bdice commented Apr 24, 2025 •

edited

Loading

bdice Apr 25, 2025 •

edited

Loading

bdice Apr 25, 2025 •

edited

Loading

bdice commented Apr 28, 2025 •

edited

Loading

bdice commented May 8, 2025 •

edited

Loading