Avoid boxing already-resolved masks in MaskFuture - #9667
Conversation
`MaskFuture` always stored its mask behind a `Shared<BoxFuture<..>>`, so a mask that is known up front still allocated a boxed, shared future, and every clone, slice, and await went through the waker machinery to hand back a value that was sitting in memory. Sparse random-access scans hit this hardest: they run without a filter, so every split's mask is ready at construction, and the struct layout reader clones it once per leaf field. Split the inner state into `Ready` and `Pending`. Ready masks resolve inline, slice eagerly, and clone without allocating; pending masks keep the previous shared-future behaviour. Measured on the nested-structs random-access benchmark (1M rows, 6 leaf fields, cached file handle), median of 5 runs x 300 takes: indices build-futures stage end-to-end take uniform 99 275.5us -> 237.5us 4958us -> 4886us correlated 100 150.2us -> 123.8us 3425us -> 3265us fixed 6 77.5us -> 71.2us 1238us -> 1161us Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
joseph-isaacs
added
the
action/bench-random-access
Claude
github-actions
Bot
removed
the
action/bench-random-access
joseph-isaacs
added
the
changelog/performance
Claude
joseph-isaacs
added
the
action/bench-random-access
github-actions
Bot
removed
the
action/bench-random-access
|
Two red checks on 45a41b7 are runner loss, not this PR.
Both are the same infrastructure event at ~16:56–16:58. Nothing to port: there is no fix to carry, since neither failure is a defect. Supporting evidence that the diff is not implicated — it only touches I attempted the one re-run for each and GitHub refused with Generated by Claude Code |
Merging this PR will regress 2 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | words_gather_scalar_avx2[65536] |
8.3 µs | 9.4 µs | -11.96% |
| ❌ | Simulation | cold_misaligned[(16, 64)] |
345.6 µs | 391.1 µs | -11.64% |
| ⚡ | WallTime | arrow_checked_add_u32_neon[16384] |
20.4 µs | 13.4 µs | +52.2% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/random-access-perf-3su1yz (45a41b7) with develop (1fe8dda)
Footnotes
-
106 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. ↩
-
4 benchmarks were run, but are now archived. If they were deleted in another branch, consider rebasing to remove them from the report. Instead if they were added back, click here to restore them. ↩
|
On the CodSpeed report: I checked all three flagged benchmarks and none of them can be attributed to this diff, which touches only Two of the three are compile-time impossible.
The third is not on a SplitBy::Layout.splits(reader.as_ref(), &(0..file.row_count()), &[FieldMask::All])That walks the layout tree via The +52.2% "improvement" is the tell. It is in Two further points of context, both visible in the report itself: CodSpeed warns I am not making a code change for these. The pending re-run of the cancelled Codspeed jobs should also produce a complete report; if Generated by Claude Code |
Polar Signals Profiling ResultsLatest Run
Powered by Polar Signals Cloud |
|
Correction to my previous comment, and a status update. Both red checks are now green, and the platform retried them itself. I was wrong about the skipped benchmarks. I wrote that the 106 skipped benchmarks were consistent with the runner shutdown. That is not right. Codspeed attempt 2 finished at 17:05:25 and the performance report was posted at 17:05:57, so the report came from the complete, successful run — not from the degraded one. Please disregard that part of my reasoning; the skipped count has some other cause. What that does and does not change. The dependency-direction argument is untouched and still rules out two of the three flagged benchmarks outright: For Generated by Claude Code |
Benchmarks: Random Access 📖Commits: PR How to read Verdict and Engines
vortex / arrow-ipc / ns (0.992x ➖, 0↑ 0↓)
random-access / vortex-file-compressed / ns (0.991x ➖, 0↑ 0↓)
random-access / parquet / ns (0.997x ➖, 0↑ 0↓)
random-access / lance / ns (0.995x ➖, 0↑ 0↓)
|
2 participants
Summary
Reduces per-split overhead on the sparse random-access path.
MaskFuturealways stored its mask behind aShared<BoxFuture<..>>, even when the mask wasalready known. Random access hits this hardest: it runs without a filter, so
split_execbuildsevery split's mask up front via
MaskFuture::ready(..), andStructReader::projection_evaluationthen clones that mask once per leaf field. Each of those clones, slices, and awaits went through
boxed-future and waker machinery to hand back a value that was already sitting in memory.
Changes
Splits the inner state into
ReadyandPending. Ready masks resolve inline, slice eagerly, andclone without allocating; pending masks keep the previous shared-future behaviour.
inspecton analready-resolved mask now fires immediately rather than deferring to the first poll, which is
documented on the method — it currently has no production callers.
Tests cover that ready and pending variants agree through
sliceandawait, and thatinspectfires for a resolved mask.
Local measurements
nested-structs(1M rows, 6 leaf fields, cached file handle), interleaved A/B of separately builtbaseline and patched binaries, median of 5 runs x 300 takes:
The saving is concentrated in future construction, which is only ~5% of a take, so the end-to-end
effect is small and may not clear benchmark noise in CI.
Why the end-to-end win is small
Instrumenting bytes actually read per
take()shows the dominant cost is read amplification ratherthan scan machinery: a 99-row point lookup reads 33.97 MiB, against 33.98 MiB of segments in the
whole file. 100 correlated rows read 26.66 MiB; even 6 rows read 11.72 MiB. The file has 54 segments
of roughly 904 KiB-1 MiB, because the default
data_block_target_bytesof 1 MiB coalesces the8192-row blocks into megabyte data blocks, so reading one row fetches its whole segment. Sampling
agrees: ~76% of CPU samples sit on the tokio blocking pool servicing those reads.
That is a write-side granularity question (
WriteStrategyBuilder::with_data_block_target_bytes)with a scan-throughput trade-off, so it is deliberately left out of this PR.
Generated by Claude Code