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

[DO NOT MERGE] Experiment with using cuda::memory_resource interface #840

Closed
wants to merge 16 commits into from

Conversation

harrism
Copy link
Member

@harrism harrism commented Aug 5, 2021

This is an experiment, opening as a PR for discussion.

Currently only stream_ordered_memory_resource<cuda::memory_kind::device> is used as a basis. So far I've converted only cuda_memory_resource and managed_memory_resource. Obviously this is not ideal because these should really have different memory kinds. And really we should be able to use a single class with different kinds for the implementation of these two. But it's a start, and it works with our current test infrastructure.

Adding other memory kinds will result in multiple independent base classes which will require refactoring how we write MR tests so that they are not value parameterized on the MR. This is a pain because value parameterization lets us create simple factory functions for instantiating the MR, which is valuable since some MRs are multi-layered (e.g. hybrid_MR) so a simple call to the ctor is not sufficient.

Suggested roadmap to integrating cuda::memory_resource (from @jrhemstad )

  • Delete device_memory_resource class
  • Make all resources that currently inherit from device_memory_resource inherit from cuda::stream_ordered_memory_resource<Kind>, where in instance of cuda_memory_resource Kind is device and for pool_memory_resource Kind is a template parameter dependent on the upstream
  • Throw if alignment is larger than 256 (full alignment support can be added later)
  • Update all RMM interfaces that currently accept device_memory_resource* to cuda::resource_view<Props...> where Props... is a variadic type pack
  • Rename device_buffer to buffer<Kind>
  • Update all libcudf (and other libs) interfaces that take device_memory_resource* to resource_view<device_accessible>

Comments (from @harrism):
This is obviously incredibly invasive to RAPIDS and other dependent libraries. We need to plan how to manage that disruption and how to break up the above into separate PRs. The final step should be doable independently, however we would need to keep device_memory_resource until that is complete.

@github-actions github-actions bot added CMake cpp Pertains to C++ code labels Aug 5, 2021
@harrism harrism changed the title [DO NOT MERGE] Experiment using cuda::memory_resource classes as base [DO NOT MERGE] Experiment with using cuda::memory_resource interface Aug 5, 2021
@harrism harrism requested a review from jrhemstad August 5, 2021 00:28
@harrism harrism added breaking Breaking change improvement Improvement / enhancement to an existing function labels Aug 23, 2021
@github-actions github-actions bot added gpuCI Python Related to RMM Python API labels Oct 5, 2021
@harrism harrism changed the base branch from branch-21.10 to branch-21.12 October 6, 2021 01:22
@robertmaynard
Copy link
Contributor

robertmaynard commented Nov 1, 2021

With cudf updated to rapids_cpm_libcudacxx you can now use that logic to simplify the process of using a custom libcudacxx in this PR.

For example to use your custom fork of libcudacxx you would write ( for both rmm and cudf ):

function(find_and_configure_libcudacxx)
  include(${rapids-cmake-dir}/cpm/libcudacxx.cmake)
  include(${rapids-cmake-dir}/cpm/package_override.cmake)

  file(WRITE ${CMAKE_BINARY_DIR}/libcudacxx.json [=[
{
  "packages" : {
    "libcudacxx" : {
      "version" : "1.5.0",
      "git_url" : "https://github.com/mzient/libcudacxx.git",
      "git_tag" : "memres_view"
    }
  }
}]=])
  rapids_cpm_package_override(${CMAKE_BINARY_DIR}/libcudacxx.json)
  rapids_cpm_libcudacxx(BUILD_EXPORT_SET rmm-exports
                          INSTALL_EXPORT_SET rmm-exports)
  set(LIBCUDACXX_INCLUDE_DIR "${libcudacxx_SOURCE_DIR}/include" PARENT_SCOPE)
endfunction()

find_and_configure_libcudacxx()

@harrism
Copy link
Member Author

harrism commented Nov 1, 2021

Thanks @robertmaynard ! #883 is the more relevant PR for this. Mentioning it here so this shows up on that PR.

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

This PR has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates. This PR will be labeled inactive-90d if there is no activity in the next 60 days.

@github-actions
Copy link

github-actions bot commented Mar 2, 2022

This PR has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change CMake cpp Pertains to C++ code gpuCI improvement Improvement / enhancement to an existing function inactive-30d inactive-90d Python Related to RMM Python API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants