Skip to content

Use cuda::mr::memory_resource instead of raw device_memory_resource#1095

Merged
rapids-bot[bot] merged 37 commits intorapidsai:branch-23.12from
miscco:memory_resource
Nov 17, 2023
Merged

Use cuda::mr::memory_resource instead of raw device_memory_resource#1095
rapids-bot[bot] merged 37 commits intorapidsai:branch-23.12from
miscco:memory_resource

Conversation

@miscco
Copy link
Contributor

@miscco miscco commented Sep 21, 2022 •
edited by harrism
Loading

This introduces cuda::mr::{async_}resource_ref as a type erased safe resource wrapper that is meant to replace uses of rmm::mr::{host, device}_memory_resource pointers.

We provide both async and classic allocate functions that delegate back to the original resource used to construct the cuda::mr::{async_}resource_ref

In comparison to {host, device}_memory_resource the new feature provides additional compile time checks that will help users avoid common pitfalls with heterogeneous memory allocations.

As a first step we provide the properties cuda::mr::host_accessible and cuda::mr::device_accessible. These properties can be added to an internal or even external type through a free function get_property

// For a user defined resource
struct my_resource {
  friend void get_property(my_resource const&, cuda::mr::device_accessible) noexcept {}
};

// For an external resource
void get_property(some_external_resource const&, cuda::mr::device_accessible) noexcept {}

The advantage is that we can constrain interfaces based on these properties

void do_some_computation_on_device(cuda::mr::async_resource_ref<cuda::mr::device_accessible> mr, ...) { ... }

This function will fail to compile if it is passed any resource that does not support async allocations or is not tagged as providing device accessible memory. In the same way the following function will only compile if the provided resource provides the classic allocate / deallocate interface and is tagged to provide host accessible memory

void do_some_computation_on_host(cuda::mr::resource_ref<cuda::mr::host_accessible> mr, ...) { ... }

The property system is highly flexible and can easily be user provided to add their own properties as needed. That gives it both the flexibility of an inheritance based implementation and the security of a strictly type checked interface

@miscco miscco requested review from a team as code owners September 21, 2022 06:08
@GPUtester
Copy link
Contributor

GPUtester commented Sep 21, 2022

Can one of the admins verify this patch?

Admins can comment ok to test to allow this one PR to run or add to allowlist to allow all future PRs from the same author to run.

@github-actions github-actions bot added CMake cpp Pertains to C++ code labels Sep 21, 2022
miscco
miscco commented Sep 21, 2022
View reviewed changes
miscco
miscco commented Sep 21, 2022
View reviewed changes
@jrhemstad
Copy link
Contributor

jrhemstad commented Sep 21, 2022

@miscco so what's your current thinking about the steps we'd take to transition rapids libraries?

Ultimately we want to get rid of the device_memory_resource base class all together. Likewise transition all rapids apis from taking a device_memory_resource* to any_resource_ref<access::device>. But we'll have to work up to that point. The transition from passing a pointer to a reference will be annoying because it means everything will need to change all at the same time.

@miscco
Copy link
Contributor Author

miscco commented Sep 21, 2022

@miscco so what's your current thinking about the steps we'd take to transition rapids libraries?

Ultimately we want to get rid of the device_memory_resource base class all together. Likewise transition all rapids apis from taking a device_memory_resource* to any_resource_ref<access::device>. But we'll have to work up to that point. The transition from passing a pointer to a reference will be annoying because it means everything will need to change all at the same time.

I guess the advantage that we have is that the new interface itself is non intrusive. So my proposal would be to expand test coverage and merge this PR right away into rmm. We can then think about which parts of rapids to move over to the new shiny.

We might also want to think about adding a new interface first and then remove the old one as a second step afterwards

@vyasr
Copy link
Contributor

vyasr commented Sep 21, 2022

Does this PR replace #883 and #840?

@miscco
Copy link
Contributor Author

miscco commented Sep 22, 2022

Does this PR replace #883 and #840?

Yes, that is the goal

miscco
miscco commented Sep 26, 2022
View reviewed changes
@harrism
Copy link
Member

harrism commented Sep 26, 2022

ok to test

@harrism
Copy link
Member

harrism commented Sep 26, 2022

@miscco can you target this at branch-22.12? We don't target main with PRs, and 22.10 is entering code freeze later this week. (RAPIDS has an interesting branching model)

@harrism harrism added feature request New feature or request breaking Breaking change labels Sep 26, 2022
@harrism
Copy link
Member

harrism commented Sep 26, 2022

You will also need to run clang-format to pass the style checks.

@harrism harrism requested review from harrism and jrhemstad and removed request for codereport and cwharris September 26, 2022 23:49
@harrism harrism mentioned this pull request Sep 27, 2022
Closed
@miscco
Copy link
Contributor Author

miscco commented Sep 28, 2022

Yeah, I am not fully ready with the device side memory resources. I hope to finish something this week

@miscco miscco changed the base branch from main to branch-22.12 September 30, 2022 09:28
@miscco miscco requested review from a team as code owners September 30, 2022 09:28
@harrism harrism mentioned this pull request Nov 7, 2023
Closed
harrism added a commit to harrism/cuspatial that referenced this pull request Nov 9, 2023
@harrism
Use artifacts from rapidsai/rmm#1095 in CI
4706ce1
@harrism harrism mentioned this pull request Nov 9, 2023
Closed
3 tasks
bdice
bdice reviewed Nov 15, 2023
View reviewed changes
@bdice
Copy link
Collaborator

bdice commented Nov 16, 2023

I discussed with @harrism -- he wants to finish testing downstream RAPIDS repos with commit 41c1bea. In the meantime, I created a branch with temporary changes from my PR review. https://github.com/bdice/rmm/tree/memory_resource-bdice

Feel free to merge those three commits into this PR when ready.

@miscco
Do not propagate the device_accessible property for containers
af12733 
We currently have no design on how we want to propagate the properties of the memory_resource, so drop this for now until have a proper design ready
bdice
bdice approved these changes Nov 16, 2023
View reviewed changes
Copy link
Collaborator

@bdice bdice left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Approving as-is. Let's merge tonight (in the next 5-6 hours, ideally) and let the nightlies rebuild all RAPIDS packages. We'll iron out any problems (like the potential issue with cuGraph) once we see if the nightlies pass.

@harrism
Copy link
Member

harrism commented Nov 17, 2023

/merge

@miscco
Copy link
Contributor Author

miscco commented Nov 17, 2023

Thanks a lot for helping me getting this in 🎉

@wence- wence- mentioned this pull request Nov 27, 2023
Closed
This was referenced Jan 30, 2024
Open
Closed
@jrhemstad jrhemstad mentioned this pull request Feb 22, 2024
Open
@miscco miscco mentioned this pull request Feb 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@robertmaynard robertmaynard robertmaynard left review comments

@harrism harrism harrism approved these changes

@bdice bdice bdice approved these changes

@jrhemstad jrhemstad jrhemstad approved these changes

Assignees

No one assigned

Labels

breaking Breaking change CMake cpp Pertains to C++ code feature request New feature or request Python Related to RMM Python API

Projects

RMM Project Board
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@miscco @GPUtester @jrhemstad @vyasr @harrism @ajschmidt8 @bdice @robertmaynard