Use configured copy buffer for Hadoop vectored reads - #15164
Conversation
8fcaad0 to
626c8ed
Compare
f9ef769 to
887912f
Compare
|
@binmahone please retarget to In addition, please add performance results to the description of the PR. |
be4c485 to
d3283e8
Compare
Signed-off-by: Hongbin Ma (Mahone) <mahongbin@apache.org>
d3283e8 to
ba4487c
Compare
|
Done. This PR targets The current GitHub compile failures are the expected dependency-order failure: CI resolves the published 26.06 JNI artifact, which does not yet contain |
|
@gerashegalov Following up here on your caller-supplied-buffer suggestion in NVIDIA/cudf-spark-jni#4765: this PR does use We considered retaining the buffer in We validated this exact implementation against 26.04.2 with five interleaved full-NDS runs per configuration, both with PerfIO disabled and enabled. The results did not reproduce a stable performance regression after the fix. The complete environment and per-run results are in #15163. |
| int copyBufferSize = conf.getInt(PARQUET_READ_ALLOCATION_SIZE, | ||
| RapidsInputFile.DEFAULT_READ_VECTORED_COPY_BUFFER_SIZE); | ||
| return new HadoopInputFile(filePath, fs, copyBufferSize); |
There was a problem hiding this comment.
Dependency on unpublished JNI artifact
Both RapidsInputFile.DEFAULT_READ_VECTORED_COPY_BUFFER_SIZE (line 49) and RapidsInputFile.readVectoredUsingCopyBuffer (line 91) are added in NVIDIA/cudf-spark-jni#4765, which has not yet been published in the 26.06 artifact. The PR description notes the two cannot find symbol compile errors are expected until that JNI change ships. This PR cannot be merged independently and must be gated on the JNI PR landing first.
| if (copyRanges.isEmpty()) { | ||
| return; | ||
| } | ||
| byte[] copyBuffer = new byte[copyBufferSize]; |
There was a problem hiding this comment.
Non-blocking for a follow-up: HadoopInputFile could allocate min(copyBufferSize, largest requested range) like the JNI default path does, instead of always allocating copyBufferSize. That keeps the current per-call lifetime and thread-safety properties, while avoiding an 8 MiB allocation for small footer/range reads.
## Description This updates the default `RapidsInputFile.readVectored` fallback to copy through an explicit byte-buffer loop and write into the destination `HostMemoryBuffer` with `setBytes`. The default fallback does not cache a scratch buffer in the interface implementation. It allocates a bounded buffer for each `readVectored` call, using the smaller of the default 8 MiB limit and the largest requested range. Callers with a well-defined lifecycle can use `RapidsInputFile.readVectoredUsingCopyBuffer(..., byte[])` to provide a caller-managed scratch buffer. This avoids routing default fallback vectored reads through `HostMemoryBuffer.copyFromStream`, whose internal copy chunk is 128 KiB. Internal validation and stream-copy helpers live in a package-private utility class so they do not expand the interface API surface beyond the caller-supplied helper. Related issue: NVIDIA/cudf-spark#15163. The dependent NVIDIA/cudf-spark#15164 uses the helper in `HadoopInputFile.readVectored`. It keeps the `GpuParquetScan` `readVectored` abstraction introduced by NVIDIA/cudf-spark#14674 while allowing Hadoop-backed fallback reads to honor `parquet.read.allocation.size`. ## Testing The current PR head (`901ca4f570cb843c814121a7698ba8b3d5896fbf`) was tested with: ```text mvn -B -Dmaven.antrun.skip=true -Dtest=RapidsInputFileTest \ test-compile surefire:test@default-test ``` Result: ```text Tests run: 8, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` The JNI artifact was then installed locally and the dependent cudf-spark head (`ba4487c750ba73cf2f2281288ca319ad7f3752be`) was packaged on an x86 host with: ```text mvn -B -pl sql-plugin -am -DskipTests \ -Dspark-rapids-jni.version=26.06.1-SNAPSHOT package ``` Result: `BUILD SUCCESS`. ## Performance Validation End-to-end performance validation was performed with the dependent NVIDIA/cudf-spark#15164. Five interleaved NDS runs did not reproduce a stable regression after the fix. See NVIDIA/cudf-spark#15164 for the complete results. --------- Signed-off-by: Hongbin Ma (Mahone) <mahongbin@apache.org>
|
build |
2 similar comments
|
build |
|
build |
|
build |
1 similar comment
|
build |
Signed-off-by: Hongbin Ma <mahongbin@apache.org>
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
There was a problem hiding this comment.
Pull request overview
This PR addresses a remote Parquet scan performance regression on Hadoop-backed filesystems (e.g., S3A fallback, GCS) by making the HadoopInputFile vectored-read fallback use a configurable copy buffer size (via parquet.read.allocation.size) and delegating the actual copy loop to the newer JNI helper introduced in the referenced JNI change.
Changes:
- Adds a
parquet.read.allocation.size-driven copy buffer size toHadoopInputFilecreation (defaulting to the JNI-provided fallback size). - Overrides
HadoopInputFile.readVectoredto allocate a per-call byte buffer and useRapidsInputFile.readVectoredUsingCopyBuffer(...)for bounded buffered reads.
| if (copyBufferSize <= 0) { | ||
| throw new IllegalArgumentException(PARQUET_READ_ALLOCATION_SIZE + " must be positive"); | ||
| } |
| } | ||
| byte[] copyBuffer = new byte[copyBufferSize]; | ||
| RapidsInputFile.readVectoredUsingCopyBuffer(this, output, copyRanges, copyBuffer); | ||
| } |
|
build |
…5242) Fixes #15163. ### Description This is the `main` forward-port of #15164, which targeted `release/26.06`. When an optimized input-file implementation is not selected, remote Parquet reads use `HadoopInputFile` and the generic `RapidsInputFile.readVectored` fallback. Before NVIDIA/cudf-spark-jni#4765, that fallback used `HostMemoryBuffer.copyFromStream` with a 128 KiB internal copy chunk, while the pre-#14674 Hadoop copy loop used `parquet.read.allocation.size` with an 8 MiB default. This PR overrides `HadoopInputFile.readVectored` to use the caller-supplied-buffer helper from the JNI `main` forward-port, NVIDIA/cudf-spark-jni#4808: - read the copy size from `parquet.read.allocation.size` - default to the JNI fallback size of 8 MiB - allocate a temporary buffer for each `readVectored` call - avoid retained shared buffers and `ThreadLocal` state - leave `GpuParquetScan` on the `inputFile.readVectored` abstraction introduced by #14674 The measured regression was on the S3A fallback with PerfIO disabled. The same `HadoopInputFile` fallback is also used by GCS and other Hadoop-backed filesystems that do not provide their own optimized `readVectored` implementation. When S3 PerfIO is enabled, `S3InputFile.readVectored` continues to use the optimized PerfIO path and bypasses this fallback. Requires NVIDIA/cudf-spark-jni#4808. The previous `release/26.06` implementation is #15164 and NVIDIA/cudf-spark-jni#4765. ### Testing The JNI `main` forward-port (`daca8177f626c4c48676beea5f16d36a974620fe`) passed all eight `RapidsInputFileTest` tests. After installing that JNI `26.08.0-SNAPSHOT` artifact locally, this commit (`5ec2c7ffc1763d5b141e20066ab303e9c287e962`) was packaged on an x86 host with: ```text mvn -B -pl sql-plugin -am -DskipTests \ -Dspark-rapids-jni.version=26.08.0-SNAPSHOT package ``` Result: `BUILD SUCCESS` for all four reactor modules. Five interleaved full-NDS runs per configuration did not reproduce a stable performance regression after the fix, with either PerfIO disabled or enabled. See #15163 for the complete environment, per-run results, and analysis. ### Checklists Documentation - [ ] Updated for new or modified user-facing features or behaviors - [x] No user-facing change Testing - [ ] Added or modified tests to cover new code paths - [ ] Covered by existing tests - [x] Not required Performance - [ ] Tests ran and results are added in the PR description - [x] Issue filed with a link in the PR description - [ ] Not required Signed-off-by: Hongbin Ma <mahongbin@apache.org>
Fixes #15163.
Description
When an optimized input-file implementation is not selected, remote Parquet reads use
HadoopInputFileand the genericRapidsInputFile.readVectoredfallback. Before NVIDIA/cudf-spark-jni#4765, that fallback usedHostMemoryBuffer.copyFromStreamwith a 128 KiB internal copy chunk, while the pre-NVIDIA/cudf-spark#14674 Hadoop copy loop usedparquet.read.allocation.sizewith an 8 MiB default.This PR overrides
HadoopInputFile.readVectoredto use the caller-supplied-buffer helper from NVIDIA/cudf-spark-jni#4765:parquet.read.allocation.sizereadVectoredcallThreadLocalstateGpuParquetScanon theinputFile.readVectoredabstraction introduced by optimize iceberg read #14674The measured regression was on the S3A fallback with PerfIO disabled. The same
HadoopInputFilefallback is also used by GCS and other Hadoop-backed filesystems that do not provide their own optimizedreadVectoredimplementation. When S3 PerfIO is enabled,S3InputFile.readVectoredcontinues to use the optimized PerfIO path and bypasses this fallback.Requires the JNI change from NVIDIA/cudf-spark-jni#4765, merged as
f5c95d88e16846a66a61870441e73c84863e706c. Therelease/26.06base branch selects JNI26.06.1-SNAPSHOTthrough #15228.Testing
The JNI change (
901ca4f570cb843c814121a7698ba8b3d5896fbf) passed all eightRapidsInputFileTesttests. After installing that JNI artifact locally, the cudf-spark change commit (ba4487c750ba73cf2f2281288ca319ad7f3752be) was packaged on an x86 host with:Result:
BUILD SUCCESS.Performance Validation
Five interleaved full-NDS runs per configuration did not reproduce a stable performance regression after the fix, with either PerfIO disabled or enabled. See #15163 for the complete environment, per-run results, and analysis.
Checklists
Documentation
Testing
Performance