Repository navigation
fix(layout): preserve shared segment wakeups during prefetch races - #11334
joseph-isaacs wants to merge 1 commit into
Conversation
Wake fetchers that race with a temporary prefetch poll after the shared future releases its polling state. Keep the handoff outside the mutex and release the gate on unwind. Add a deterministic regression test that forces the lost-wakeup interleaving. Signed-off-by: Joe Isaacs <[email protected]>
Polar Signals Profiling ResultsLatest Run
Powered by Polar Signals Cloud |
Benchmarks: PolarSignals Profiling 📖Commits: PR datafusion / vortex-file-compressed / ns (1.019x ➖, 1↑ 2↓)
|
Benchmarks: TPC-H SF=1 on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.994x ➖, 0↑ 0↓)
datafusion / parquet / ns (0.997x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.010x ➖, 0↑ 0↓)
duckdb / parquet / ns (1.002x ➖, 0↑ 0↓)
|
Benchmarks: TPC-H SF=10 on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.994x ➖, 0↑ 0↓)
datafusion / parquet / ns (1.000x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.998x ➖, 0↑ 0↓)
duckdb / parquet / ns (0.993x ➖, 0↑ 0↓)
|
Benchmarks: FineWeb NVMe 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.014x ➖, 0↑ 1↓)
datafusion / parquet / ns (0.998x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.013x ➖, 0↑ 1↓)
duckdb / parquet / ns (1.006x ➖, 0↑ 0↓)
|
Benchmarks: TPC-DS SF=1 on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.003x ➖, 0↑ 0↓)
datafusion / parquet / ns (1.000x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.995x ➖, 0↑ 0↓)
duckdb / parquet / ns (0.994x ➖, 0↑ 0↓)
|
Benchmarks: Clickbench on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.991x ➖, 0↑ 0↓)
datafusion / parquet / ns (0.994x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.996x ➖, 0↑ 0↓)
duckdb / parquet / ns (0.990x ➖, 0↑ 0↓)
|
Benchmarks: TPC-H SF=1 on S3 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.193x ➖, 0↑ 4↓)
datafusion / parquet / ns (1.031x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.036x ➖, 1↑ 2↓)
duckdb / parquet / ns (1.036x ➖, 0↑ 0↓)
|
Benchmarks: TPC-DS SF=10 on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.001x ➖, 0↑ 1↓)
datafusion / parquet / ns (0.996x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.002x ➖, 1↑ 1↓)
duckdb / parquet / ns (1.002x ➖, 0↑ 0↓)
|
Summary
A temporary V2 prefetch poll can consume a shared segment notification while concurrent readers return pending, leaving those readers without another wakeup. Preserve the handoff so readers can resume after the active poll finishes.
Based on
developatb2f8d0f946e0(s00).Changes
b2f8d0f946e0and passes ona2765d424380, using the same regression-test source in both runs.Benchmark evidence
This is a correctness/liveness repair. Its evidence is the deterministic before/after regression test; no latency win is claimed. The full-suite screen separately measures its cost.
32-core aarch64 host, full 100-file partitioned ClickBench fixture, Vortex V2, ordinary local reads with no mmap, 32 workers/partitions/threads, DuckDB
--reuse. DataFusion 55.2 retainsenable_topk_aggregation=true. Compiler, Cargo feature union, profile and runtime environment are fixed across parent/candidate. Executable and library hashes, canonical SQL and fixture identities are recorded.Full43 screening
The complete series screen finished 168/168 processes, with no failures or overlaps and successful source/SQL/binary/fixture checks. Each source/engine comparison used four balanced exact-parent process pairs and two matching identical-binary A/A pairs, with 10 executions/query and the first 3 discarded. This screen is descriptive: no new gain, equivalence, or regression-free claim is inferred, and no observations or controls were subtracted.
The deterministic regression test supports this wakeup correctness fix independently of performance. The all43 screen nevertheless shows DuckDB Q2 medians 2.73% slower in all four pairs and Q42 means 10.21% slower with variable tails; the latter's medians are mixed. Broad CPU and total latency changes are small and control-sized. Treat this as a correctness-value change with disclosed measured costs, not a performance win or neutrality proof; optional follow-up can resolve the cost decision.
Validation
scan::v2::io::tests,segments::shared::tests.--locked, therelease_debugprofile,profile.release_debug.debug=1, andRUSTFLAGS="-C target-cpu=native -C force-frame-pointers=yes". Rust changes were formatted with pinned nightly; patch whitespace checks passed. Workspace-wide clippy was not run; CI status is separate.Measured local source commit:
a2765d42438062db73d431ef6d296930e69f5b28; full source tree:7b8fcf0df070cccdf14e57cad26a467edf0faa10. Measured parent:b2f8d0f946e003d3e463d1447ed357ed88990686.PR source commit:
da10665e5b0fad748a1e0e6550cc19880edd0317; mapped parent:b2f8d0f946e003d3e463d1447ed357ed88990686. The complete source tree and mapped parent graph match the measured source. GitHub regenerated commit metadata/timestamps, so the measured local commit and executable hashes remain the benchmark identities. The complete remote Git object, original message and author/committer signoffs were verified.Measurements use the develop snapshot
b2f8d0f946e003d3e463d1447ed357ed88990686. Develop advanced during the campaign; these results retain the recorded source and binary identities.Repository SQL benchmark
The SQL workflow completed 8/8 suites on this exact PR head. The ClickBench bot verdict is No clear signal (low confidence).
The preset covers V2 ClickBench, TPC-H, TPC-DS and FineWeb, plus a legacy PolarSignals control. These cloud runs use suite-specific historical develop baselines and
FLAT_LAYOUT_INLINE_ARRAY_NODE=1; they are separate from the local exact-parent comparisons and cannot isolate this stack's incremental effect. Ordinary draft CI was skipped; SQL benchmark success is not an overall CI pass.