perf(vindex): configure independent range-read concurrency - #720
perf(vindex): configure independent range-read concurrency#720jerry-024 wants to merge 4 commits into
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.
Purpose
Decouple Vindex range-read I/O concurrency from global-index execution concurrency so each limit can be tuned independently.
Brief change log
global-index.range-read-thread-numtable option with a default of32.Tests
cargo +1.97.0 fmt --all -- --checkcargo +1.97.0 test -p paimon vindex::range_reader(16 passed)API and Format
Adds the
global-index.range-read-thread-numtable option. There is no storage-format change.Notes
32pending the 10M A/B benchmark result.