perf(vindex): decouple range-read limits and remove chunk barrier - #720
Conversation
f242b52 to
3ce7998
Compare
0946fcd to
3ec12da
Compare
shyjsarah
left a comment
There was a problem hiding this comment.
Thanks for separating Vindex range-read concurrency and for the thorough runtime/concurrency tests. I found one scalability/fairness issue that should be addressed before merge, plus one documentation follow-up.
| .push(merged.len()); | ||
| } | ||
| let ranges: Vec<_> = merged.iter().map(|merged| merged.range.clone()).collect(); | ||
| let fetched = self.fetch_range_batch(&ranges)?; |
There was a problem hiding this comment.
[Major] Please keep the refill pipeline bounded instead of queueing every merged range at once.
try_join_all creates/polls a future for every range, so the semaphore limits only active FileRead calls—not queued permit waiters or completed Bytes retained until the slowest read finishes. Since Tokio's semaphore is FIFO, one large pread can also enqueue all of its ranges ahead of other Vindex readers sharing this limiter. This changes the old chunk barrier into O(R) queued state and can cause cross-reader head-of-line blocking.
Could this use a rolling FuturesUnordered/buffer_unordered(max_range_read_concurrency) window, carrying the original index to restore result order? That preserves immediate refill while keeping local waiters bounded. A regression test with two cloned readers sharing permits would also help verify that a large batch cannot monopolize the queue.
| } | ||
|
|
||
| /// Maximum number of concurrent range reads shared by Vindex readers in one | ||
| /// search operation. This is independent of [`Self::global_index_thread_num`]. |
There was a problem hiding this comment.
[Minor] Please update the existing option documentation and migration notes for this new independent limit.
The current global_index_thread_num Rust docs still say it limits global-index/PK-vector I/O and that 1 gives strict sequential execution; docs/src/sql.md also documents only global-index.thread-num. Those statements are no longer true for Vindex range reads when this option is absent (the new default is 32). Please narrow the old option's documented scope, document global-index.range-read-thread-num and its default, and call out the changed upgrade behavior for tables with a non-default global-index.thread-num.
| const FULL_TEXT_INDEX_SEARCH_MODE_OPTION: &str = "full-text-index.search-mode"; | ||
| const GLOBAL_INDEX_ROW_COUNT_PER_SHARD_OPTION: &str = "global-index.row-count-per-shard"; | ||
| const GLOBAL_INDEX_THREAD_NUM_OPTION: &str = "global-index.thread-num"; | ||
| const GLOBAL_INDEX_RANGE_READ_THREAD_NUM_OPTION: &str = "global-index.range-read-thread-num"; |
There was a problem hiding this comment.
global-index.vindex.read-thread-num
shyjsarah
left a comment
There was a problem hiding this comment.
I re-checked the latest range-read pipeline. The earlier unbounded queue/statistics issues are addressed, but I found two remaining gaps around the response budget and cross-reader fairness.
| Ok(results.pop().expect("one requested range")) | ||
| let mut result = None; | ||
| self.fetch_range_batch(std::slice::from_ref(&range), |_, data| { | ||
| result = Some(data); |
There was a problem hiding this comment.
[Major] The single-range path releases the response permit before the payload is actually copied.
fetch_range_batch keeps the response permit only until consume returns, but this closure moves the owned Bytes into result and returns immediately. read_one performs copy_from_slice afterwards (and may retain the same Bytes in the scalar cache), when the response permit has already been released. Consequently, cloned readers using single-range pread can hold more than the intended global 2C responses at once; the existing bounded-response test blocks inside the callback and therefore does not cover this escape path.
Could we either copy into the caller buffer inside the protected callback, or return an RAII value that binds Bytes to its OwnedSemaphorePermit until the copy/cache handoff finishes? A regression test with C=1 and three clones doing single-range reads would make the intended bound explicit.
| })) | ||
| .await; | ||
| let _ = sender.send(fetched); | ||
| .buffer_unordered(response_limit); |
There was a problem hiding this comment.
[Major] The current two-stage admission does not reliably preserve the cloned-reader fairness asserted by the test.
Each batch independently polls up to 2C work items, first acquiring a response permit and only then entering the I/O semaphore. Ordering can be lost between those two queues: a clone may obtain/wait for response admission but not reach the I/O queue before the original batch's refill work does. I reproduced cloned_reader_is_not_queued_behind_an_entire_batch failing once in the full range-reader suite, while subsequent runs passed. The test hook also signals before acquire_owned() is known to be Pending, so it does not establish the ordering assumed by the assertion.
Please make the test notify only after the response acquire is actually pending. If cross-reader fairness is a required contract, admission needs to preserve ordering across the response→I/O transition (for example through a shared scheduler/queue); otherwise the test and documentation should state the weaker bounded-delay guarantee rather than assert a specific start order.
There was a problem hiding this comment.
Thanks for catching this. Strict cross-clone start ordering is not a contract of this optimization. These clones are internal readers within the same Vindex search, and the limiter is intended to enforce the global resource bounds (C active I/O operations and at most 2C retained responses), not deterministic scheduling between clones.
Adding a shared scheduler solely to preserve start order would add contention and complexity without benchmark evidence that it improves query tail latency. I therefore removed the flaky ordering test and its test-only hook. The implementation intentionally makes no cross-clone start-order guarantee; if benchmarks later show starvation or a meaningful tail-latency problem, we can add a fair scheduler in a separate, measured change.
| const FULL_TEXT_INDEX_SEARCH_MODE_OPTION: &str = "full-text-index.search-mode"; | ||
| const GLOBAL_INDEX_ROW_COUNT_PER_SHARD_OPTION: &str = "global-index.row-count-per-shard"; | ||
| const GLOBAL_INDEX_THREAD_NUM_OPTION: &str = "global-index.thread-num"; | ||
| const GLOBAL_INDEX_RANGE_READ_THREAD_NUM_OPTION: &str = "global-index.vindex.read-thread-num"; |
There was a problem hiding this comment.
Please rename const GLOBAL_INDEX_RANGE_READ_THREAD_NUM_OPTION too.
e9ec171 to
fb07f1e
Compare
* main: perf: vectorize raw vector search (apache#734) feat(file_index): add predicate evaluation foundation (apache#721) feat(go): add postpone fixed-bucket write bindings (apache#722) perf(vindex): split build timing logs by phase (apache#723) fix(avro): read TIME, BLOB, MULTISET and non-string-key map columns (apache#724) fix(datafusion): surface tag create-time and retention in $tags (apache#728) [core] Support multivalue global index (apache#731) feat: add Java-compatible array predicate pushdown (apache#732) fix: serialize unbounded varchar as string (apache#730) perf(vindex): decouple vector read threads and remove chunk barrier (apache#720) feat(vindex): add DiskANN and IVF-SQ/RQ support (apache#726) # Conflicts: # crates/paimon/src/table/data_file_reader.rs # crates/paimon/src/table/vindex_index_build_builder.rs
Purpose
Decouple Vindex range-read I/O concurrency from global-index execution concurrency and remove the range chunk barrier. This keeps storage reads busy when one range is slow while bounding retained responses.
Brief change log
global-index.vindex.read-thread-numtable option with a default of64, matching Lance.C) from the response prefetch limit (2C) to preserve throughput without retaining every response.Benchmark
Release-mode range-read microbenchmark on Apple Silicon, reported as the median of 3 sequential runs. Both revisions use 4 KiB non-coalescing ranges and an explicitly configured concurrency limit of 64, matching the new table-option default.
Because
main(ab90d01) does not contain this benchmark, the same benchmark harness was backported tomainsolely for comparison.main(ab90d01)The straggler workload gives normal reads 1 ms latency and every 64th read 10 ms latency. Peak in-flight reads remained 64 on both revisions, so the gain comes from rolling submission across the former chunk boundary rather than higher I/O concurrency. The hot-cache workload did not materially regress.
Reproduce after building in release mode:
cargo test -p paimon --lib vindex_range_read_benchmark --release -- --ignored --nocaptureThe separate 10M end-to-end A/B benchmark was run at concurrency 32 and completed with 29.9% higher QPS and unchanged Recall/NDCG.
Tests
cargo +1.97.0 fmt --all -- --checkcargo +1.97.0 test -p paimon vindex::range_reader(20 passed, 1 ignored)cargo +1.97.0 test -p paimon --lib global_index_range_read_thread_num(2 passed)cargo test -p paimon --lib vindex_range_read_benchmark --release -- --ignored --nocapture(3 sequential runs per revision at C=64)API and Format
Adds the
global-index.vindex.read-thread-numtable option with a default of64. There is no storage-format change.