search

rapidsai / raft

RAFT contains fundamental widely-used algorithms and primitives for machine learning and information retrieval. The algorithms are CUDA-accelerated and form building blocks for more easily writing high performance applications.
https://docs.rapids.ai/api/raft/stable/
Apache License 2.0
784 stars 195 forks source link

Do not initialize the pinned mdarray at construction time #2478

Closed achirkin closed 2 weeks ago

achirkin commented 3 weeks ago

thrust::host_vector initializes its elements at creation and requires the element type be default-constructible. This translates to raft::pinned_mdarray and makes the mdarray unusable for non-default-constructible objects, like cuda::atomic<> (and many user-defined types). This is against all other mdarray types in raft, which are based on rmm::device_uvector and are not initialized at construction time. The PR changes the underlying container to a plain pointer + cudaMallocHost/cudaFreeHost.

Breaking change: if anyone relies on the pinned_mdarray to initialize itself, the code will break (but mdarrays should not initialize at construction in raft anyway). The affected classes have different private members now, so the ABI changes as well.

achirkin commented 3 weeks ago

CC @cjnolet @benfred do you have any objections to this? I may be missing some context on why the previous implementation was based on thrust.

achirkin commented 3 weeks ago

The problem is encountered in https://github.com/rapidsai/cuvs/pull/261, which uses a custom container as a workaround at the moment.

achirkin commented 2 weeks ago

Thanks, that's a good point! I've looked at the RMM resources and there were two pinned memory resources, so I was not sure which not to use. But perhaps more importantly, they have the stream semantics in their API, which is a bit misleading in the context of pinned arrays, which are created on the host and do not offer any stream ordering.

achirkin commented 2 weeks ago

It's also worth noting, that we don't need the .resize() functionality for the mdarrays, which makes the implementation extremely simple compared to RMM.

achirkin commented 2 weeks ago

/merge