perf(arrow-select): add take_record_batch_unchecked to skip redundant bounds checks - #10945
perf(arrow-select): add take_record_batch_unchecked to skip redundant bounds checks#10945Rich-T-kid wants to merge 6 commits into
Conversation
|
run benchmark take_kernel |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/introduce-take_record_batch_unchecked (47e2de0) to 27a6a40 (merge-base) diff Run configurationrun benchmark take_kernelBENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench take_kernel File an issue against this benchmark runner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Kubernetes messageFile an issue against this benchmark runner |
|
run benchmark take_kernel |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/introduce-take_record_batch_unchecked (47e2de0) to 27a6a40 (merge-base) diff Run configurationrun benchmark take_kernelBENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench take_kernel File an issue against this benchmark runner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Kubernetes messageFile an issue against this benchmark runner |
| .map(|c| take(c, indices, None)) | ||
| .collect::<Result<Vec<_>, _>>()?; | ||
| RecordBatch::try_new(record_batch.schema(), columns) | ||
| unsafe { take_record_batch_unchecked(record_batch, indices) } |
There was a problem hiding this comment.
i think this is only safe when doing it on subsequent columns after the first column; the idea being that after doing (checked) take on the first column, we know for the next columns all indices are within bounds (otherwise wouldve panicked) since invariant of record batch is all columns are of same length
There was a problem hiding this comment.
ah this is my mistake, was rushing when I pushed this and forgot to revert this. this was only placed here to compare the existing benchmarks.
the point of introducing the unsafe take_record_batch_unchecked was to allow users to opt into the unsafe code clearly, forcing them to add in unsafe blocks to their code.
will remove
There was a problem hiding this comment.
ill push the update after we can view the benchmark results, from @adriangbot
|
seems like take kernels are too big by now to run all at once; we'll need to run with filters 🤔 |
|
run benchmark take_kernel |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/introduce-take_record_batch_unchecked (47e2de0) to 27a6a40 (merge-base) diff Run configurationrun benchmark take_kernel
env:
BENCH_FILTER: "record batch"BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench take_kernel File an issue against this benchmark runner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Kubernetes messageFile an issue against this benchmark runner |
|
run benchmark take_kernel |
|
Hi @Rich-T-kid, your benchmark configuration could not be parsed (#10945 (comment)). Error: Usage: Any benchmark name is accepted: Per-side configuration ( env:
# shared env is inherited by BOTH the build and the run, so build
# flags go here. Builds default to no debuginfo for speed; opt back
# in for hung-job gdb dumps and cap jobs to stay within memory:
CARGO_PROFILE_RELEASE_DEBUG: "1"
CARGO_BUILD_JOBS: "1"
baseline:
ref: v45.0.0
env:
# per-side env only reaches the benchmark run, not the build
DATAFUSION_RUNTIME_MEMORY_LIMIT: 1G
changed:
ref: v46.0.0
env:
DATAFUSION_RUNTIME_MEMORY_LIMIT: 2GFile an issue against this benchmark runner |
|
run benchmark take_kernel |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/introduce-take_record_batch_unchecked (47e2de0) to 27a6a40 (merge-base) diff Run configurationrun benchmark take_kernel
env:
BENCH_FILTER: "take_record_batch"BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench take_kernel File an issue against this benchmark runner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Kubernetes messageFile an issue against this benchmark runner |
|
run benchmark take_kernel |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/introduce-take_record_batch_unchecked (47e2de0) to 27a6a40 (merge-base) diff Run configurationrun benchmark take_kernel
env:
BENCH_FILTER: "take_record_batch"BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench take_kernel File an issue against this benchmark runner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Kubernetes messageFile an issue against this benchmark runner |
47e2de0 to
176446c
Compare
|
show benchmark queue |
|
Hi @Rich-T-kid, you asked to view the benchmark queue (#10945 (comment)). No pending jobs. File an issue against this benchmark runner |
|
run benchmark take_kernel |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/introduce-take_record_batch_unchecked (176446c) to 6e728ce (merge-base) diff Run configurationrun benchmark take_kernel
env:
BENCH_FILTER: "take_record_batch"BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench take_kernel File an issue against this benchmark runner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Kubernetes messageFile an issue against this benchmark runner |
|
run benchmark take_kernel |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/introduce-take_record_batch_unchecked (03d57b2) to 027b45f (merge-base) diff Run configurationrun benchmark take_kernel
env:
BENCH_FILTER: "take_record_batch"BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench take_kernel File an issue against this benchmark runner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Kubernetes messageFile an issue against this benchmark runner |
#10945 <- here, this PR is stacked on top of #10944
#10944
Which issue does this PR close?
takekernels #8879.Rationale for this change
take_record_batchpreviously called take per column, which always runs a bounds check on every index even when the caller already knows the indices are valid. For workloads doing repeated record batch takes with pre-validated indices — such as sort, merge, or filter pipelines — this check is redundant and measurable overhead. Benchmarks show ~13% speedup on primitive-column batches at 1024 rows when the check is skipped.What changes are included in this PR?
take_impl::<_, false>directlyAre these changes tested?
yes, existing test + miri
Are there any user-facing changes?
new
take_record_batch_unchecked()method.