Invert layout writers to push API - #9423
Open
gatesn wants to merge 4 commits into
Open
Conversation
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Merging this PR will degrade performance by 6.91%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | cold_misaligned[(64, 256)] |
4.4 ms | 5.5 ms | -19.86% |
| ❌ | Simulation | take[small_m/shuffled/primitive/nonnull/chunks=16384/indices=16] |
1.1 ms | 1.2 ms | -11.12% |
| ⚡ | WallTime | words_gather_scalar[65536] |
9.4 µs | 8.3 µs | +13.27% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ngates/push-layout-writer (2b83616) with develop (b363fb7)
Footnotes
-
46 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Signed-off-by: Nicholas Gates <nick@nickgates.com>
gatesn
added
the
action/bench-compress
github-actions
Bot
removed
the
action/bench-compress
gatesn
marked this pull request as ready for review
August 15, 2026 02:18
gatesn
enabled auto-merge (squash)
August 15, 2026 02:18
Contributor
Polar Signals Profiling ResultsLatest Run
Powered by Polar Signals Cloud |
onursatici
added
the
action/bench-compress
github-actions
Bot
removed
the
action/bench-compress
onursatici
added
the
action/bench-compress
github-actions
Bot
removed
the
action/bench-compress
onursatici
reviewed
Aug 18, 2026
Comment on lines
+250
to
+251
| writer.finish(sequence_id).await?; | ||
| return writer.close().await; |
Contributor
There was a problem hiding this comment.
Calling finish and close here kind of invalides what the layout writer trait says. If we have a layout writer actor per sibling layouts, then with this implementation we can't guarantee all sibling finish calls will be done before starting calling close. Maybe we should get Close as an actor message?
| impl LayoutWriter for CollectLayoutWriter { | ||
| async fn write(&mut self, sequence_id: SequenceId, chunk: ArrayRef) -> VortexResult<()> { | ||
| let reservation = self.buffered_bytes.reserve(chunk.nbytes()); | ||
| self.chunks.push((sequence_id, chunk, reservation)); |
Contributor
There was a problem hiding this comment.
This would deadlock, we shouldn't hold sequence_id's like this, We can store the chunks but we should only store the latest sequence_id
| buffer_size: u64, | ||
| buffered_bytes: crate::BufferedBytesTracker, | ||
| pending: Option<(SequenceId, ArrayRef, BufferedBytesReservation)>, | ||
| chunks: VecDeque<(SequenceId, ArrayRef, BufferedBytesReservation)>, |
Contributor
There was a problem hiding this comment.
similarly, storing old sequence id's would deadlock because it will block all sibling writers
| /// This avoids accounting mismatches when interior-mutable arrays (e.g. `SharedArray`) | ||
| /// change their reported size after being pushed. | ||
| data: VecDeque<(ArrayRef, u64)>, | ||
| data: VecDeque<(SequenceId, ArrayRef, u64, BufferedBytesReservation)>, |
Contributor
There was a problem hiding this comment.
similarly this is deadlock prone as well, same reason.
I did reproduce this with using:
StructStrategy
├── field a: BufferedStrategy
│ └── ChunkedLayoutStrategy
│ └── FlatLayoutStrategy
└── field b: ChunkedLayoutStrategy
└── FlatLayoutStrategy
As an example, but it can also deadlock with the default strategy if the columns compress differently and the difference exceeds the buffer capacity
myrrc
added
changelog/break
github-actions
Bot
removed
the
action/bench-all
Contributor
Benchmarks: String Encoding 📖vortex / vortex-file-compressed / ms (0.978x ➖, 1↑ 0↓)
vortex / vortex-file-compressed / % (1.000x ➖, 0↑ 0↓)
|
Contributor
|
How do I migrate with this break? |
Contributor
Benchmarks: PolarSignals Profiling 📖Vortex (geomean): 0.993x ➖ datafusion / vortex-file-compressed / ns (0.993x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: FineWeb NVMe 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.992x ➖, 0↑ 1↓)
datafusion / vortex-compact / ns (0.985x ➖, 0↑ 0↓)
datafusion / parquet / ns (0.995x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.078x ➖, 2↑ 4↓)
duckdb / vortex-compact / ns (1.078x ➖, 0↑ 1↓)
duckdb / parquet / ns (1.001x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: TPC-DS SF=1 on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.000x ➖, 2↑ 1↓)
datafusion / vortex-compact / ns (0.999x ➖, 1↑ 3↓)
datafusion / parquet / ns (1.000x ➖, 0↑ 1↓)
duckdb / vortex-file-compressed / ns (1.003x ➖, 4↑ 8↓)
duckdb / vortex-compact / ns (0.989x ➖, 3↑ 4↓)
duckdb / parquet / ns (1.014x ➖, 3↑ 8↓)
File Size Changes (8 files changed, -0.0% overall, 4↑ 4↓)
Totals:
|
Contributor
Benchmarks: Clickbench Sorted on NVME 📖Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.955x ➖, 3↑ 1↓)
datafusion / vortex-compact / ns (0.944x ➖, 3↑ 1↓)
datafusion / parquet / ns (1.018x ➖, 1↑ 1↓)
duckdb / vortex-file-compressed / ns (0.920x ➖, 3↑ 0↓)
duckdb / vortex-compact / ns (0.933x ➖, 2↑ 0↓)
duckdb / parquet / ns (1.001x ➖, 0↑ 0↓)
File Size Changes (200 files changed, -0.0% overall, 102↑ 98↓)
Totals:
|
Contributor
Benchmarks: Statistical and Population Genetics 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
duckdb / vortex-file-compressed / ns (1.032x ➖, 2↑ 3↓)
duckdb / vortex-compact / ns (0.931x ➖, 4↑ 1↓)
duckdb / parquet / ns (1.007x ➖, 0↑ 0↓)
File Size Changes (1 files changed, -0.0% overall, 0↑ 1↓)
Totals:
|
Contributor
Benchmarks: FineWeb S3 📖Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.901x ➖, 1↑ 0↓)
datafusion / vortex-compact / ns (0.990x ➖, 1↑ 1↓)
datafusion / parquet / ns (1.061x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.106x ➖, 0↑ 2↓)
duckdb / vortex-compact / ns (1.027x ➖, 0↑ 0↓)
duckdb / parquet / ns (1.005x ➖, 0↑ 0↓)
|
Contributor
Benchmarks: Clickbench on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.997x ➖, 1↑ 1↓)
datafusion / vortex-compact / ns (0.995x ➖, 0↑ 0↓)
datafusion / parquet / ns (0.985x ➖, 1↑ 0↓)
duckdb / vortex-file-compressed / ns (1.005x ➖, 0↑ 4↓)
duckdb / vortex-compact / ns (0.993x ➖, 2↑ 1↓)
duckdb / parquet / ns (0.990x ➖, 2↑ 0↓)
File Size Changes (91 files changed, +0.0% overall, 56↑ 35↓)
Totals:
|
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
4 participants
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.
Summary
write,finish, andcloseSequenceIdprotocol so ordered byte sinks remain deterministic while key/value sinks may ignore orderingbuffered_bytes()Motivation
The previous public push writer fed a
SendableArrayStreaminto a background stream-based strategy tree. This made incremental writing indirect and made strategy composition and buffered-byte ownership harder to follow.The push inversion keeps one independently driven child per structural field and restores the old sequence identifiers to preserve deterministic on-disk segment ordering. Structural fan-out and finish poll sibling children concurrently, so ordered segment output does not depend on a sibling driver that is not being advanced.
Backpressure matches the previous writer's memory model: the public/root input, every structural child edge, and the segment-buffer sink each have capacity one. A slow
VortexWritefills the segment sink first, then leaf and structural mailboxes, and ultimately makes publicWriter::writeawait. Compression retains its separate, explicitly bounded CPU-concurrency window. Queued and in-flight arrays remain byte-accounted until the receiving writer has processed them.The stream compatibility adapter also explicitly drops its unused sequence pointer before finishing, preventing it from holding an ordering predecessor indefinitely.
Behavior
Writer::bytes_written()reports bytes passed to the underlyingVortexWrite.Writer::buffered_bytes()reports logical bytes retained by layout strategies and asynchronous work; it excludes output-sink buffering and allocator/statistics overhead.VortexWriteimplementations retain control over sink flushing.Performance
Release in-memory fixture: 16 numeric columns, 8 chunks, 16K rows/chunk, 16 MiB logical input. Each run used two warmups and ten measured iterations against the latest merged
develop; each value below is the median of its ten measured iterations.developrun mediansdevelop→ pushMean 1-to-8-worker scaling was 3.10x on
developand 3.65x with the push writer.Validation
cargo test -p vortex-layout -p vortex-filecargo check -p vortex-layout -p vortex-file -p vortex-cuda -p vortex-benchcargo clippy --all-targets --all-featurescargo +nightly fmt --allgit diff --checkThe layout tests include mailbox saturation/backpressure, cross-field compression overlap with deterministic segment IDs, buffered-byte accounting for queued and spawned work, nested structural ordering, dictionary codes-before-values, and zone maps after data.