rmm icon indicating copy to clipboard operation
rmm copied to clipboard

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

Open harrism opened this issue 4 years ago • 4 comments

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.

harrism avatar Aug 05 '21 00:08 harrism

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()

robertmaynard avatar Nov 01 '21 17:11 robertmaynard

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

harrism avatar Nov 01 '21 21:11 harrism

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[bot] avatar Dec 02 '21 21:12 github-actions[bot]

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.

github-actions[bot] avatar Mar 02 '22 22:03 github-actions[bot]