Convert device_memory_resource* to device_async_resource_ref #2269
Conversation
harrism
added
improvement
There was a problem hiding this comment.
Thanks for the PR! There are plenty build failures atm (see below).
In the meanwhile, what's your opinion regarding get_workspace_resource, which didn't make it into this PR? From the RMM design point of view, shall we better change it to return the resource_ref (thus, losing the information about its type), or keep returning the pointer? Note, we provide extra helpers to get the limits from the limiting adapter.
|
I will have to dig in when I'm back on Monday. It all built successfully on my local copy. |
|
@achirkin I didn't have those failures locally because apparently in the dev container
I don't want to tackle this until we redesign |
|
Figured out how to build benchmarks. |
There was a problem hiding this comment.
I've checked that the PR doesn't change the current behavior w.r.t. usage of memory resources anywhere in raft, LGTM. Also thanks for fixing numerous mischiefs like unnecessary dynamic or c-style casts.
I don't want to tackle this until we redesign rmm::set_current_device_resource / rmm::get_current_device_resource, because that redesign can inform this one.
Could you please share, in which direction you plan to redesign these?
FWIW, this is another place where RAFT does things differently than every other RAPIDS library. Consistency would be good.
It may be doing things even more differently pretty soon. Please, have a look at the problem we're trying to solve: #2194 . If you have any suggestions on how we can solve the issues outlined in that PR more in line with other RAPIDS libraries, your feedback would be very appreciated!
| configured_raft_resources(configured_raft_resources&&) = delete; | ||
| configured_raft_resources& operator=(configured_raft_resources&&) = delete; |
There was a problem hiding this comment.
Did you mark this constructor as deleted only because it defaulted to deleted already due to raft::device_resources member, or there was a more specific/design issue?
If the former, I'd prefer to wrap the device_resources into a unique pointer to keep the move constructor (we move it in benchmarks when initializing algo wrappers). But I can do it later in a follow-on PR.
There was a problem hiding this comment.
Yes exactly. clang-tidy complains that this is explicitly defaulted even though it is implicitly deleted due to the deleted ctors in the res_ member.
If you are going to have these clang-tidy settings, why ignore the warnings? Wrapping the device resources in a unique ptr is up to you -- it's outside of my scope and the scope of this PR.
I think I can revert all changes to this file. Turns out the resource_ref header is no longer used.
There was a problem hiding this comment.
Actually it should include device_memory_resource.hpp in order to correctly IWYU.
|
/merge |
Re-enable move constructor for the `configured_raft_resources`. It was implicitly deleted before, which was exposed and made explicit in #2269 . Allowing move semantics here means avoiding an extra unwanted overhead during algorithm preparation in the benchmarks tool. Authors: - Artem M. Chirkin (https://github.com/achirkin) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2311
Description
Closes #2261
For reviewers:
Many of changes are simple textual replace of
rmm::mr::device_memory_resource *withrmm::device_async_resource_ref. However there are several places where RAFT used a default value ofnullptrfordevice_memory_resource*parameters. This is incompatible with aresource_ref, which is a lightweight non-owning reference class, not a pointer. In most places, I was able to either remove the default parameter value, or usermm::mr::get_current_device_resource(). In the case of ivf_pq, I removed the deprecated versions ofsearchthat took anmrparameter.I removed the unused old src/util/memory_pool.cpp and its headers.