Remove bridge infrastructure and device_memory_resource#2324
Remove bridge infrastructure and device_memory_resource#2324bdice merged 4 commits intorapidsai:stagingfrom
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
bdice
added
breaking
Delete device_memory_resource.hpp and device_memory_resource_view.hpp.
Strip DMR bridge code from cccl_adaptors.hpp, keeping shared_resource_cast
wrappers. Inline do_allocate/do_deallocate into allocate/deallocate in
stream_ordered_memory_resource. Convert benchmarks from shared_ptr<DMR> to
any_device_resource. Rewrite test mocks to satisfy CCCL concepts directly,
with copyable forwarding wrappers to work around basic_any type-erasure
limitations with GMock types. Replace reinterpret_cast stream constructions
with cuda_stream_view{}.
bdice
force-pushed
the
delete-device-memory-resource
branch
from
f8c7621 to
6d1e1ee
Compare
There was a problem hiding this comment.
Goodbye friend, you've been a great header for so many applications. 🫶
bdice
force-pushed
the
delete-device-memory-resource
branch
from
454e3ad to
7f2b7f1
Compare
bdice
requested review from
davidwendt and
wence-
and removed request for
a team,
davidwendt and
wence-
wence-
left a comment
wence-
left a comment
There was a problem hiding this comment.
Mostly good with one comment around the alignment propagation
| @@ -83,11 +80,11 @@ inline constexpr bool is_cccl_async_resource_ref_v<cccl_async_resource_ref<R>> = | |||
|
|
|||
| /** | |||
| * @brief A wrapper around CCCL synchronous_resource_ref that adds compatibility with | |||
There was a problem hiding this comment.
question: Do these shims go away completely in a followup?
| RMM_EXPECTS( | ||
| alignment <= rmm::CUDA_ALLOCATION_ALIGNMENT && rmm::is_supported_alignment(alignment), | ||
| "Alignment must be less than or equal to 256 and a power of two", | ||
| rmm::bad_alloc); |
There was a problem hiding this comment.
Note (probably followup). Previously every allocation with a requested alignment would go through this method, giving us a central place to raise if the requested alignment was not satisfiable.
Now that this indirection is gone we need to:
- Make sure that all MRs explicitly pass alignment through to any upstream (not currently done today)
- Ensure all "leaf" MRs validate the alignment parameter.
Point (1) should not be done by the aligned resource adaptor (the whole point is it can change the alignment), but should be done by everyone else.
There was a problem hiding this comment.
Yes, excellent callout. I knew this problem existed but I've been deferring on fixing it. There are a bunch of issues related to alignment already, and I planned to tackle it as a part of those. For now I filed a new issue issue #2336 to ensure this isn't forgotten.
2 participants
Summary
device_memory_resource.hppanddevice_memory_resource_view.hppper_device_resourceAPIs and bridge helperscccl_adaptors.hpp(remove DMR bridge code, retain wrapper for deletion in a follow-up)mock_resource.hpp,device_check_resource_adaptor.hpp) to use CCCL concepts directlycallback_memory_resource, aligned, arena, and failure_callback testsCloses #2296
Part of #2011