Experiment with StreamEncoder::encode_to - #10985
Draft
Phoenix500526 wants to merge 1 commit into
Draft
Conversation
A direct Write path lets us measure the cost of returning buffers and forwarding them separately before settling the public sink design. Refs apache#10445
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 draft explores whether allowing
StreamEncoderto write to aborrowed destination can reduce the overhead of returning a
Vec<Buffer>and then forwarding each buffer separately.
The motivation is primarily performance and memory behavior, but this is
an experiment rather than a claim that the API is ready or that it provides
a broad performance improvement. In particular, making the encoder accept
Writealso makes it overlap more withStreamWriterand weakens thesans-I/O boundary.
What changes are included in this PR?
StreamEncoder::encode_to<W: Write + ?Sized>.finish_to<W: Write + ?Sized>.ZSTD-compressed streams.
encode + writeandencode_toCriterion benchmarks, with andwithout ZSTD compression.
Are these changes tested?
Yes. The following checks pass locally:
A local Criterion run produced these 95% confidence intervals:
encode + writeencode_toThe uncompressed intervals did not overlap in this run. The ZSTD intervals
overlap and do not demonstrate an improvement. These are results from one
local machine, so I do not want to claim a stable percentage improvement.
Benchmark command:
cargo bench -p arrow-ipc --bench ipc_writer --features zstd -- \ 'StreamEncoder/(encode_and_write_10|encode_to_10)'Important limitations of this experiment:
WriteintoVec<u8>; it does not model anasync writer or backpressure.
Vec<EncodedBuffer>internally, sothis does not implement the owned-buffer sink proposed in the issue.
discarded; this is documented but not represented by a poisoned state.
Are there any user-facing changes?
This adds public methods without changing existing behavior. The API shape,
boundary, and naming are deliberately open for discussion while this is a
draft.
@alamb, CC'ing you because this follows your suggestion in #10277. I would
appreciate your feedback on whether this synchronous
Writeprototype is auseful boundary, whether an owned-buffer sink would better preserve the
sans-I/O design, and whether the benchmark should include allocation or
memory measurements before proceeding.