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

Deprecate and remove get_mem_info methods from memory resources. #1388

Closed
4 tasks done
Tracked by #16 ...
harrism opened this issue Nov 23, 2023 · 0 comments · Fixed by #1519
Closed
4 tasks done
Tracked by #16 ...

Deprecate and remove get_mem_info methods from memory resources. #1388

harrism opened this issue Nov 23, 2023 · 0 comments · Fixed by #1519
Assignees
Labels
cpp Pertains to C++ code feature request New feature or request tech debt debt Internal clean up and improvements to reduce maintenance and technical debt in general

Comments

@harrism
Copy link
Member

harrism commented Nov 23, 2023

device_memory_resource::get_mem_info has always been hard to support for arbitrary memory resources. Sometimes it is impossible to calculate and so MRs have another method supports_get_mem_info() that returns true if get_mem_info() returns useful values.

We are now in the process of adopting cuda::memory_resource and resource_ref concepts into RMM, and we would like to embrace the flexibility and composability they enable. cuda::memory_resource does not provide anything like get_mem_info(), just like its inspiration std::pmr::memory_resource.

I am now experimenting with defining a host_pinned_memory_resource using the cuda::memory_resource concept and not deriving from rmm::mr::device_memory_resource. This works as an upstream for other resources, such as pool_memory_resource, except for one detail: pool_memory_resource relies on cudaMemGetInfo if the initial and maximum pool sizes are not specified, but that function cannot be used for pinned host memory!

I propose that we deprecate and remove the use of supports_get_mem_info() and get_mem_info(). For resources like pool, I recommend that we change them to NOT query memory sizes and make initial and maximum pool sizes required constructor parameters. While this may seem to make them harder to use, we can expose rmm::detail::available_device_memory() (which calls cudaMemGetInfo) as a utility in the public interface to use when the user knows the pool is device memory. We may also be able to provide a utility function that estimates host memory available.

Once we move entirely to the cuda::memory_resource concept, we will be able to drop the device_memory_resource base class and in that way remove the deprecated get_mem_info methods.

See also #305

@harrism harrism added feature request New feature or request tech debt debt Internal clean up and improvements to reduce maintenance and technical debt in general cpp Pertains to C++ code labels Nov 23, 2023
@harrism harrism self-assigned this Nov 23, 2023
rapids-bot bot pushed a commit that referenced this issue Jan 18, 2024
…ool_memory_resource`. (#1392)

Depends on #1417

Adds a new `host_pinned_memory_resource` that implements the new `cuda::mr::memory_resource` and `cuda::mr::async_memory_resource` concepts which makes it usable as an upstream MR for `rmm::mr::device_memory_resource`. 

Also tests a pool made with this new MR as the upstream.

Note that the tests explicitly set the initial and maximum pool sizes as using the defaults does not currently work. See #1388 .

Closes #618

Authors:
  - Mark Harris (https://github.com/harrism)
  - Lawrence Mitchell (https://github.com/wence-)

Approvers:
  - Michael Schellenberger Costa (https://github.com/miscco)
  - Alessandro Bellina (https://github.com/abellina)
  - Lawrence Mitchell (https://github.com/wence-)
  - Jake Hemstad (https://github.com/jrhemstad)
  - Bradley Dice (https://github.com/bdice)

URL: #1392
rapids-bot bot pushed a commit that referenced this issue Jan 23, 2024
…nfo() nonvirtual. Remove derived implementations and calls in RMM (#1430)

Closes #1426

As part of #1388, this PR contributes to deprecating and removing all `get_mem_info` functionality from memory resources.  This first PR makes these methods optional without deprecating them. 

 - Makes `rmm::mr::device_memory_resource::supports_get_mem_info()` nonvirtual (and always return false) 
 - Makes `rmm::mr::device_memory_resource::do_get_mem_info()` nonvirtual (and always return `{0, 0}`).
 - Removes all derived implementations of the above.
 - Removes all calls to the above.

Authors:
  - Mark Harris (https://github.com/harrism)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #1430
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this issue Jan 24, 2024
Part of rapidsai/rmm#1388. This removes now-optional and soon-to-be deprecated functions from cuDF's custom device_memory_resource implementations:
 * `supports_get_mem_info()`
 * `do_get_mem_info()`

Authors:
  - Mark Harris (https://github.com/harrism)

Approvers:
  - Jason Lowe (https://github.com/jlowe)
  - Nghia Truong (https://github.com/ttnghia)
  - Vukasin Milovanovic (https://github.com/vuule)

URL: #14832
rapids-bot bot pushed a commit to rapidsai/raft that referenced this issue Jan 24, 2024
Part of rapidsai/rmm#1388. This removes now-optional and soon-to-be deprecated functions from cuDF's custom device_memory_resource implementations:

 - `supports_get_mem_info()`
 - `do_get_mem_info()`

Authors:
  - Mark Harris (https://github.com/harrism)
  - Corey J. Nolet (https://github.com/cjnolet)

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

URL: #2108
PointKernel pushed a commit to PointKernel/cudf that referenced this issue Jan 25, 2024
…14832)

Part of rapidsai/rmm#1388. This removes now-optional and soon-to-be deprecated functions from cuDF's custom device_memory_resource implementations:
 * `supports_get_mem_info()`
 * `do_get_mem_info()`

Authors:
  - Mark Harris (https://github.com/harrism)

Approvers:
  - Jason Lowe (https://github.com/jlowe)
  - Nghia Truong (https://github.com/ttnghia)
  - Vukasin Milovanovic (https://github.com/vuule)

URL: rapidsai#14832
rapids-bot bot pushed a commit that referenced this issue Jan 26, 2024
…s_get_mem_info(). (#1436)

Closes #1427 . Part of #1388.

#1430 made these functions non-virtual and removed them from all MRs and tests. This PR completes the next step of deprecating them.  

Merge after 
 - rapidsai/raft#2108
 - rapidsai/cudf#14832

Authors:
  - Mark Harris (https://github.com/harrism)

Approvers:
  - Lawrence Mitchell (https://github.com/wence-)
  - Michael Schellenberger Costa (https://github.com/miscco)
  - Bradley Dice (https://github.com/bdice)

URL: #1436
@rapids-bot rapids-bot bot closed this as completed in #1519 Apr 5, 2024
@rapids-bot rapids-bot bot closed this as completed in 48820cb Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp Pertains to C++ code feature request New feature or request tech debt debt Internal clean up and improvements to reduce maintenance and technical debt in general
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

1 participant