Skip to content

Commit 65f8363

Browse files
authored
Fix use-after-destroy and stream ordering in Parquet IO utils (NVIDIA#22529)
This PR fixes the use-after-destroy and stream ordering (with PTDS input) issue (with host buffer source) in the `fetch_byte_ranges_to_device_async` IO utility used by parquet and hybrid scan. See follow up PR NVIDIA#22550 that reduces the locked region size by moving all `host_read_async` outside it. Authors: - Muhammad Haseeb (https://github.com/mhaseeb123) Approvers: - Bradley Dice (https://github.com/bdice) - Amin Aramoon (https://github.com/aminaramoon) - Vukasin Milovanovic (https://github.com/vuule) URL: NVIDIA#22529
1 parent 2a452f4 commit 65f8363

1 file changed

Lines changed: 51 additions & 29 deletions

File tree

‎cpp/src/io/parquet/io_utils/parquet_io_utils.cpp‎

Lines changed: 51 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#include "io/comp/common.hpp"
77
#include "io/parquet/parquet_common.hpp"
88

9+
#include <cudf/detail/utilities/cuda_memcpy.hpp>
910
#include <cudf/detail/utilities/host_worker_pool.hpp>
1011
#include <cudf/detail/utilities/integer_utils.hpp>
1112
#include <cudf/io/datasource.hpp>
@@ -21,6 +22,7 @@
2122
#include <cuda/std/tuple>
2223

2324
#include <numeric>
25+
#include <tuple>
2426

2527
/**
2628
* @file parquet_io_utils.cpp
@@ -89,6 +91,9 @@ fetch_byte_ranges_to_device_async(
8991
std::vector<size_t> io_offsets;
9092
std::vector<size_t> io_sizes;
9193
std::vector<uint8_t*> destinations;
94+
io_offsets.reserve(byte_ranges.size());
95+
io_sizes.reserve(byte_ranges.size());
96+
destinations.reserve(byte_ranges.size());
9297

9398
for (size_t chunk = 0; chunk < byte_ranges.size();) {
9499
auto const io_offset = static_cast<size_t>(byte_ranges[chunk].offset());
@@ -110,12 +115,24 @@ fetch_byte_ranges_to_device_async(
110115
CUDF_EXPECTS(io_offsets.size() == io_sizes.size() and io_sizes.size() == destinations.size(),
111116
"Unexpected number of IO offsets, sizes, or destinations");
112117

118+
using host_read_buffer = std::unique_ptr<cudf::io::datasource::buffer>;
119+
120+
// Vectors to hold futures from datasource
113121
std::vector<std::future<size_t>> device_read_tasks{};
114-
std::vector<std::future<size_t>> host_read_tasks{};
115-
device_read_tasks.reserve(byte_ranges.size());
116-
host_read_tasks.reserve(byte_ranges.size());
122+
std::vector<std::future<host_read_buffer>> host_read_tasks{};
123+
device_read_tasks.reserve(io_offsets.size());
124+
host_read_tasks.reserve(io_offsets.size());
125+
126+
// Vectors to store intermediate host read buffers and relevant pointers
127+
std::vector<void*> copy_dsts{};
128+
std::vector<size_t> copy_sizes{};
129+
copy_dsts.reserve(io_offsets.size());
130+
copy_sizes.reserve(io_offsets.size());
131+
132+
// Vector to store intermediate host buffers
133+
std::vector<host_read_buffer> host_buffers{};
117134

118-
// device_read_async is not guaranteed to follow stream-ordering (see datasource API docs).
135+
// `device_read_async` is not guaranteed to follow stream-ordering (see datasource API docs)
119136
stream.synchronize();
120137

121138
{
@@ -133,35 +150,40 @@ fetch_byte_ranges_to_device_async(
133150
device_read_tasks.emplace_back(
134151
datasource.device_read_async(io_offset, io_size, dest, stream));
135152
} else {
136-
// Read the column chunk data to the host buffer copy it to the device buffer
137-
host_read_tasks.emplace_back(cudf::detail::host_worker_pool().submit_task(
138-
[&datasource, io_offset, io_size, dest, stream]() {
139-
auto host_buffer = datasource.host_read(io_offset, io_size);
140-
cudf::detail::cuda_memcpy_async(
141-
cudf::device_span<uint8_t>{dest, io_size},
142-
cudf::host_span<uint8_t const>{host_buffer->data(), io_size},
143-
stream);
144-
return io_size;
145-
}));
153+
// Asynchronously read column chunk data to a host buffer
154+
host_read_tasks.emplace_back(datasource.host_read_async(io_offset, io_size));
155+
copy_dsts.push_back(static_cast<void*>(dest));
156+
copy_sizes.push_back(io_size);
146157
}
147158
});
148-
}
149159

150-
auto sync_function = [](decltype(host_read_tasks) host_read_tasks,
151-
decltype(device_read_tasks) device_read_tasks) {
152-
for (auto& task : host_read_tasks) {
153-
task.get();
154-
}
155-
for (auto& task : device_read_tasks) {
156-
task.get();
160+
// If there are host reads, schedule a batched memcpy to device
161+
if (not host_read_tasks.empty()) {
162+
std::vector<void const*> copy_srcs{};
163+
copy_srcs.reserve(host_read_tasks.size());
164+
host_buffers.reserve(host_read_tasks.size());
165+
166+
for (auto& task : host_read_tasks) {
167+
host_buffers.emplace_back(task.get());
168+
copy_srcs.push_back(host_buffers.back().get()->data());
169+
}
170+
CUDF_CUDA_TRY(cudf::detail::memcpy_batch_async(
171+
copy_dsts.data(), copy_srcs.data(), copy_sizes.data(), copy_dsts.size(), stream));
157172
}
158-
};
159-
return {std::move(column_chunk_buffers),
160-
std::move(column_chunk_data),
161-
std::async(std::launch::deferred,
162-
sync_function,
163-
std::move(host_read_tasks),
164-
std::move(device_read_tasks))};
173+
174+
// Synchronize the stream if `memcpy_batch_async` was scheduled to safely discard the host
175+
// buffers
176+
if (not host_buffers.empty()) { stream.synchronize(); }
177+
178+
auto sync_function = [](decltype(device_read_tasks) device_read_tasks) {
179+
for (auto& task : device_read_tasks) {
180+
task.get();
181+
}
182+
};
183+
return {std::move(column_chunk_buffers),
184+
std::move(column_chunk_data),
185+
std::async(std::launch::deferred, sync_function, std::move(device_read_tasks))};
186+
}
165187
}
166188

167189
} // namespace cudf::io::parquet

0 commit comments

Comments
 (0)