Pinned vector factory that uses the global pool #15895
Conversation
github-actions
bot
added
libcudf
| @@ -27,6 +27,7 @@ | |||
| #include "io/utilities/parsing_utils.cuh" | |||
|
|
|||
| #include <cudf/detail/utilities/cuda.cuh> | |||
| #include <cudf/detail/utilities/logger.hpp> | |||
There was a problem hiding this comment.
used to be included indirectly, same as cpp/src/io/orc/reader_impl_chunking.cu
| @@ -380,7 +382,7 @@ thrust::host_vector<T> make_host_vector_async(device_span<T const> v, rmm::cuda_ | |||
| * @brief Asynchronously construct a `std::vector` containing a copy of data from a device | |||
| * container | |||
| * | |||
| * @note This function synchronizes `stream`. | |||
| * @note This function does not synchronize `stream`. | |||
There was a problem hiding this comment.
Out of the scope of the current PR, the _sync suffix in make_host_vector_sync is wordy and confusing and not aligned with CUDA/C++ naming convention: APIs are treated as synchronous by default and only asynchronous ones are named by adding a _async suffix. I assume this is also why the typo was introduced in the first place.
| @@ -19,6 +19,7 @@ | |||
| #include <cudf/utilities/default_stream.hpp> | |||
| #include <cudf/utilities/error.hpp> | |||
|
|
|||
| #include <rmm/aligned.hpp> | |||
There was a problem hiding this comment.
used to be included indirectly
| @@ -32,7 +33,7 @@ namespace { | |||
|
|
|||
| struct host_ticket { | |||
| cudaEvent_t event; | |||
| cudf::detail::pinned_host_vector<char> buffer; | |||
| std::unique_ptr<cudf::detail::rmm_host_vector<char>> buffer; | |||
There was a problem hiding this comment.
there's an array of host_tickets, so it needs to have a default constructor
There was a problem hiding this comment.
moved to a separate file with plans to add other user-facing APIs related to pinned memory.
No real changes to the code, just move + rebrand to pinned
…fea-pinned-vector-factory
…fea-pinned-vector-factory
|
@abellina I changed the API namespace and replaced host with pinned to reflect the new requirement; updated the Java side until CI passed, please review when you get a chance. |
…fea-pinned-vector-factory
github-actions
bot
added
Python
github-actions
bot
removed
Python
cpp/src/utilities/pinned_memory.cpp
Outdated
| static_assert(cuda::mr::resource_with<fixed_pinned_pool_memory_resource, | ||
| cuda::mr::device_accessible, | ||
| cuda::mr::host_accessible>, | ||
| ""); |
There was a problem hiding this comment.
Should this have a message? This would be a compile-time error, right?
vuule
added
5 - Ready to Merge
|
/merge |
Description
closes #15612
Expanded the set of vector factories to cover pinned vectors. The functions return
cudf::detail::host_vector, which use a type-erased allocator, allowing us to utilize the runtime configurable global pinned (previously host) resource.The
pinned_host_vectortype has been removed as it can only support the non-pooled pinned allocations. Its use is not replaced withcudf::detail::host_vector.Moved the global host (now pinned) resource out of cuIO and changed the type to host_device. User-specified resources are now required to allocate device-accessible memory. The name has been changed to pinned to reflect the new requirement.
Checklist