Remove unnecessary synchronization (miss-sync) during Parquet reading (Part 4: vector_factories) #20120

JigaoLuo · 2025-09-26T10:58:28Z

Description

For issue #18967, this PR is the first part of merging the PR Draft #18968. In this PR, I added host-pinned vector construction in vector_factories.hpp. After a careful read-through, I’ve improved the comments in this file as well.
(As discussed, I’ve also made manual changes to reduction.cuh and page_data.cu.)

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

Signed-off-by: Jigao Luo <jigao.luo@outlook.com>

This reverts commit c7ad2e8.

Signed-off-by: Jigao Luo <jigao.luo@outlook.com>

copy-pr-bot · 2025-09-26T10:58:31Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

JigaoLuo · 2025-09-26T11:01:05Z

I’ve marked this as a draft to remind myself to run the script and count how many pageable copies this PR eliminates before merging.

JigaoLuo · 2025-09-27T18:21:08Z

cpp/include/cudf/detail/utilities/vector_factories.hpp

This links back to the draft PR for reference:
https://github.com/rapidsai/cudf/pull/18968/files#diff-b281f280563cbbee7c16afb29ef989d808476a355c9c36a8f4e27fc5dc2ca4fd

JigaoLuo · 2025-09-27T18:26:40Z

cpp/include/cudf/reduction/detail/reduction.cuh

This links back to the draft PR for reference, but covering the full change in reduction.cuh:

https://github.com/rapidsai/cudf/pull/18968/files#diff-d99740825ef0d2e73c3e8392d06ca11b229400d864913b4221f3f3626ad95f85

JigaoLuo · 2025-09-27T18:29:22Z

cpp/include/cudf/reduction/detail/reduction.cuh

+  auto pinned_initial = cudf::detail::make_pinned_vector_async<OutputType>(1, stream);
+  pinned_initial[0] = initial_value;
  using ScalarType         = cudf::scalar_type_t<OutputType>;
-  auto result              = std::make_unique<ScalarType>(initial_value, true, stream, mr);
+  auto result = std::make_unique<ScalarType>(pinned_initial[0], true, stream, mr);


As we discussed on Slack: assign initial_value to element zero of a pinned vector, effectively treating it like a pinned scalar.

I forgot most of the context here :(
are we passing the value by reference here?

JigaoLuo · 2025-09-27T18:32:06Z

cpp/src/io/parquet/page_data.cu

This links back to the draft PR for reference: https://github.com/rapidsai/cudf/pull/18968/files#diff-672b345ca18d5957ee39bd7802bbf67f454cb941ab78174fbab3dc1ddaa048b3

This is the only host-to-host copy in the Parquet component (I haven’t reviewed other code paths yet). So we’ve decided to handle it in-place rather than introducing a dedicated host-to-host copy utility function in the factory.

JigaoLuo · 2025-09-27T18:35:32Z

cpp/src/io/parquet/page_data.cu

+  auto host_pinned_offsets = cudf::detail::make_host_vector<size_type>(offsets.size(), stream);
+  auto host_pinned_buff_addrs = cudf::detail::make_host_vector<size_type*>(buff_addrs.size(), stream);


As discussed on Slack, we’re using host_vector to eliminate pageable copies by setting LIBCUDF_ALLOCATE_HOST_AS_PINNED_THRESHOLD to a sufficiently high value.

I was wondering whether we should explicitly use make_pinned_vector(size, stream) here instead.

I think here what we actually want to do is change the type of the owner of offsets and buff_addrs to cudf::detail::host_vector at the call site of write_final_offsets, so we don't have to do a H2H copy.

That’s true—I lost track of the context a bit as well. Sorry for the delay; it’s been long enough :/

vuule · 2025-09-30T22:21:24Z

cpp/include/cudf/detail/utilities/vector_factories.hpp

+template <typename T>
+host_vector<T> make_pinned_vector(device_span<T const> source_data, rmm::cuda_stream_view stream)
+{
+  auto result = make_pinned_vector_async(source_data.size(), stream);


I assume this is supposed to be

Suggested change

auto result = make_pinned_vector_async(source_data.size(), stream);

auto result = make_pinned_vector_async(source_data, stream);

vuule · 2025-09-30T22:28:00Z

cpp/include/cudf/reduction/detail/reduction.cuh

+  auto pinned_initial = cudf::detail::make_pinned_vector_async<OutputType>(1, stream);
+  pinned_initial[0] = initial_value;
  using ScalarType         = cudf::scalar_type_t<OutputType>;
-  auto result              = std::make_unique<ScalarType>(initial_value, true, stream, mr);
+  auto result = std::make_unique<ScalarType>(pinned_initial[0], true, stream, mr);


I forgot most of the context here :(
are we passing the value by reference here?

vuule · 2025-09-30T22:41:31Z

cpp/src/io/parquet/page_data.cu

+  auto host_pinned_offsets = cudf::detail::make_host_vector<size_type>(offsets.size(), stream);
+  auto host_pinned_buff_addrs = cudf::detail::make_host_vector<size_type*>(buff_addrs.size(), stream);


I think here what we actually want to do is change the type of the owner of offsets and buff_addrs to cudf::detail::host_vector at the call site of write_final_offsets, so we don't have to do a H2H copy.

JigaoLuo added 10 commits September 2, 2025 20:32

make comment consistent

4c860ec

Signed-off-by: Jigao Luo <jigao.luo@outlook.com>

use cudf::detail::host_vector in comments

28bb730

Signed-off-by: Jigao Luo <jigao.luo@outlook.com>

first draft of vector_factories (no H2H copy so far)

2febabb

Signed-off-by: Jigao Luo <jigao.luo@outlook.com>

cache changes

c7ad2e8

Signed-off-by: Jigao Luo <jigao.luo@outlook.com>

Merge branch 'branch-25.10' into no-miss-sync-pinned-factory

64f98b5

Revert "cache changes"

2a1e294

This reverts commit c7ad2e8.

address the only one H2H copy

4ccf8d9

Signed-off-by: Jigao Luo <jigao.luo@outlook.com>

update comment during reading

b2c4e0c

Signed-off-by: Jigao Luo <jigao.luo@outlook.com>

remove a H2H util

81acfd5

Signed-off-by: Jigao Luo <jigao.luo@outlook.com>

reduce initial_valued pinned

c2feb39

Signed-off-by: Jigao Luo <jigao.luo@outlook.com>

JigaoLuo requested a review from a team as a code owner September 26, 2025 10:58

JigaoLuo requested review from mhaseeb123 and nvdbaranec September 26, 2025 10:58

github-actions bot assigned JigaoLuo Sep 26, 2025

github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Sep 26, 2025

JigaoLuo marked this pull request as draft September 26, 2025 11:00

Merge branch 'branch-25.12' into no-miss-sync-pinned-factory

045e9aa

JigaoLuo commented Sep 27, 2025

View reviewed changes

vuule reviewed Sep 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove unnecessary synchronization (miss-sync) during Parquet reading (Part 4: vector_factories) #20120

Remove unnecessary synchronization (miss-sync) during Parquet reading (Part 4: vector_factories) #20120

Uh oh!

JigaoLuo commented Sep 26, 2025

Uh oh!

copy-pr-bot bot commented Sep 26, 2025

Uh oh!

JigaoLuo commented Sep 26, 2025

Uh oh!

JigaoLuo Sep 27, 2025

Uh oh!

JigaoLuo Sep 27, 2025

Uh oh!

JigaoLuo Sep 27, 2025

Uh oh!

vuule Sep 30, 2025

Uh oh!

JigaoLuo Sep 27, 2025

Uh oh!

JigaoLuo Sep 27, 2025

Uh oh!

vuule Sep 30, 2025

Uh oh!

JigaoLuo Oct 1, 2025

Uh oh!

vuule Sep 30, 2025

Uh oh!

vuule Sep 30, 2025

Uh oh!

vuule Sep 30, 2025

Uh oh!

Uh oh!

		auto host_pinned_offsets = cudf::detail::make_host_vector<size_type>(offsets.size(), stream);
		auto host_pinned_buff_addrs = cudf::detail::make_host_vector<size_type*>(buff_addrs.size(), stream);

	auto result = make_pinned_vector_async(source_data.size(), stream);
	auto result = make_pinned_vector_async(source_data, stream);

Remove unnecessary synchronization (miss-sync) during Parquet reading (Part 4: vector_factories) #20120

Are you sure you want to change the base?

Remove unnecessary synchronization (miss-sync) during Parquet reading (Part 4: vector_factories) #20120

Uh oh!

Conversation

JigaoLuo commented Sep 26, 2025

Description

Checklist

Uh oh!

copy-pr-bot bot commented Sep 26, 2025

Uh oh!

JigaoLuo commented Sep 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!