perf: avoid cloning ByteView buffer lists in take and filter - #10708
Open
YimingQiao wants to merge 1 commit into
Open
perf: avoid cloning ByteView buffer lists in take and filter#10708YimingQiao wants to merge 1 commit into
YimingQiao wants to merge 1 commit into
Conversation
YimingQiao
marked this pull request as draft
August 16, 2026 16:00
YimingQiao
force-pushed
the
perf/share-byte-view-buffer-list
branch
from
August 16, 2026 16:01
c884697 to
46c989e
Compare
YimingQiao
marked this pull request as ready for review
August 16, 2026 16:02
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
This change follows directly from the existing ByteView design history:
StringViewArray::slice()andBinaryViewArray::slice()are slow (they allocate) #6408 traced slow ByteView slicing to allocating and cloning the backing-buffer list. The discussion specifically identifiedtakeas another operation that would benefit from sharing this list.Arc<[Buffer]>instead of rawVec<Buffer>inGenericByteViewArrayfor fasterslice#6427 distinguished two related but separate concerns: very large buffer lists may indicate missing GC or deduplication, but avoiding allocation when an operation retains the complete list is independently worthwhile.GenericByteViewArrayto store the collection asArc<[Buffer]>, making it possible to share the complete list without allocation.The downstream impact is concrete. apache/datafusion#16206 describes hash joins where concatenating build-side batches produces ByteView payload columns with many backing buffers, and constructing join output repeatedly calls
take. In that pattern, cloning and later dropping every buffer handle can become a significant part of execution time. Arrow issue #10692 provides a compact multi-stagetakeandBatchCoalescerreproduction of the same ownership-metadata amplification.take_byte_viewandfilter_byte_viewdo not yet use this shared representation. They rebuild the collection withdata_buffers().to_vec(), allocating a new collection and cloning everyBuffer. This makes the ownership bookkeeping for selection O(number of backing buffers), despite retaining exactly the same complete buffer list.Selection already copies the chosen views while leaving their payloads zero-copy. The cost of retaining the unchanged backing-buffer collection should therefore not grow with the number of entries in that collection. This PR completes that narrow part of the earlier design while leaving buffer canonicalization as a separate problem.
What changes are included in this PR?
GenericByteViewArray::data_buffers_shared() -> Arc<[Buffer]>to obtain shared ownership of the backing-buffer collection in O(1).takeandfilterkernels instead of rebuilding the collection.takebenchmark that varies the number of backing-buffer entries.data_buffers()remains the borrowed inspection API returning&[Buffer]. The new method returns shared ownership directly, rather than exposing&Arc<[Buffer]>and requiring callers to know that they must callArc::clone. ReturningArc<[Buffer]>is consistent with the existinginto_parts()return type and the types accepted by the ByteView array constructors.This PR only removes repeated ownership-metadata cloning. It retains the same complete collection of backing buffers as before. It does not prune or deduplicate buffer entries, remap views, run GC, or copy string/binary payloads. The broader buffer-fragmentation problem described in #10692 remains separate.
Are these changes tested?
cargo fmt --all -- --check cargo test -p arrow-array -p arrow-select cargo clippy -p arrow-array -p arrow-select --all-targets --all-features -- -D warnings cargo doc -p arrow-array --no-depsThe new Criterion benchmark takes 8,192 views distributed across a varying number of buffer entries. The entries share one immutable payload allocation, isolating collection-ownership cost from payload size.
mainand this PR were built in separate Cargo target directories on an Intel Xeon Platinum 8474C:mainExisting normal-size benchmarks did not regress:
maintake stringview 512take stringview 1024filter context mixed string view (kept 1/2)Are there any user-facing changes?
There is one additive public method,
GenericByteViewArray::data_buffers_shared. ByteView selection results now share the immutable backing-buffer collection instead of allocating an equivalent collection. Logical values, null handling, buffer indexes, payload lifetimes, and GC behavior are unchanged.AI assistance
I used OpenAI Codex to help inspect the relevant implementation history, draft the patch and tests, and prepare the benchmark harness and PR text. I reviewed the implementation and benchmark methodology and ran the checks above locally.