Add resource_ref versions of get/set_current_device_resource #1598
Conversation
Fixes #1444 Refactors mr_ref_tests.hpp and the .cpp files that include it to use a hierarchy of factory classes in which the base class contains a `resource_ref` and the derived classes are templated on the actual resource type and own the MR under test to maintain its lifetime. This way the tests can still be value parameterized (by name) and the test machinery can be type erased. In the future, if CCCL adds an `any_resource` type-erased owning resource container, we can use that to simplify this machinery. Note that until #1598 merges, this PR necessarily disables (comments out) tests of `set_current_device_resource()`. This is because the test only has a resource_ref, so it can't `set_current_device_resource(ref).` #1598 adds `set_current_device_resource_ref()` which reenables this testing. Authors: - Mark Harris (https://github.com/harrism) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) - Bradley Dice (https://github.com/bdice) URL: #1589
harrism
force-pushed
the
fea-get_current_device_resource_ref
branch
from
af5998e to
a9555c5
Compare
harrism
added
non-breaking
| * `get_current_device_resource_ref`, `set_current_device_resource_ref`, and | ||
| * `reset_current_device_resource_ref` functions provide the same functionality as their | ||
| * `device_memory_resource` counterparts, but with `device_async_resource_ref` objects. | ||
| * The raw pointer versions are expected to be deprecated and removed in a future release. |
There was a problem hiding this comment.
question: I think, from my reading, that one should either exclusively use the resource_ref set of functions, or the old (to be deprecated) versions.
That is, if one library does set_per_device_resource and another calls get_per_device_resource_ref then the setting from set_per_device_resource is not observed.
Should we mention that here?
There was a problem hiding this comment.
Yes! Coincidentally I thought of this when I stepped away for a while. Indeed they are completely separate. I could plausibly make set_per_device_resource() also update the ref_map but the other way is not possible.
Really, the intent is to deprecate it anyway.
There was a problem hiding this comment.
Added mention of non-interchangeability.
Co-authored-by: Lawrence Mitchell <wence@gmx.li>
Co-authored-by: Lawrence Mitchell <wence@gmx.li>
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
…ism/rmm into fea-get_current_device_resource_ref
|
Setting do not merge because we need to plan out the refactoring process better first. |
|
/merge |
Description
Fixes #1597
Adds new
get_per_device_resource_ref(),set_per_device_resource_ref()andcurrent_deviceversions of these, intended to replaceget_per_device_resource(). The new functions deal indevice_async_resource_ref, while the old functions deal indevice_memory_resourcepointers. Tests are updated to use the new functions.Note that I have also added
reset_per_device_resource_ref()andreset_current_device_resource_ref()which are necessary because previously we implemented the resetting behavior by passingnullptrtoset_current_device_resource(), which doesn't work withresource_refbecause it can't refer tonullptr(no such thing as a null reference).Updates tests to cover the new functionality, and re-enables tests of
set_current_device_resource_ref()which were disabled in #1589.Checklist