Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: scan concurrently in RowSeqScanExecutor #12783

Closed
st1page opened this issue Oct 11, 2023 · 7 comments
Closed

perf: scan concurrently in RowSeqScanExecutor #12783

st1page opened this issue Oct 11, 2023 · 7 comments
Milestone

Comments

@st1page
Copy link
Contributor

st1page commented Oct 11, 2023

When no key ranges predicate on the iterator, the code will create as few as possible numbers of iterator, which could decrease the scan concurrency. So when scanning the whole table, it could be bounded by the iterator.

let raw_key_ranges = if !ordered
&& matches!(encoded_key_range.start_bound(), Unbounded)
&& matches!(encoded_key_range.end_bound(), Unbounded)
{
// If the range is unbounded and order is not required, we can create a single iterator
// for each continuous vnode range.
// In this case, the `vnode_hint` must be default for singletons and `None` for
// distributed tables.
assert_eq!(vnode_hint.unwrap_or(DEFAULT_VNODE), DEFAULT_VNODE);
Either::Left(self.vnodes.vnode_ranges().map(|r| {
let start = Included(Bytes::copy_from_slice(&r.start().to_be_bytes()[..]));
let end = end_bound_of_prefix(&r.end().to_be_bytes());
assert_matches!(end, Excluded(_) | Unbounded);
(start, end)
}))
} else {

@lmatz
Copy link
Contributor

lmatz commented Oct 11, 2023

Yes, multiple iterators are better than the explicit parallelism adjustment in #12741, with less change.

@BugenZhao
Copy link
Member

BugenZhao commented Oct 11, 2023

Actually, this only affects the concurrency for creating the iterators, which has been confirmed (IIRC) to be CPU-intensive in most cases. For actual scanning, we're always in a single-threaded manner.

let iter = match iterators.len() {
0 => unreachable!(),
1 => iterators.into_iter().next().unwrap(),
// Concat all iterators if not to preserve order.
_ if !ordered => futures::stream::iter(iterators).flatten(),
// Merge all iterators if to preserve order.
_ => merge_sort(iterators.into_iter().map(Box::pin).collect()),
};

@st1page
Copy link
Contributor Author

st1page commented Oct 11, 2023

Actually, this only affects the concurrency for creating the iterators, which has been confirmed (IIRC) to be CPU-intensive in most cases. For actual scanning, we're always in a single-threaded manner.

let iter = match iterators.len() {
0 => unreachable!(),
1 => iterators.into_iter().next().unwrap(),
// Concat all iterators if not to preserve order.
_ if !ordered => futures::stream::iter(iterators).flatten(),
// Merge all iterators if to preserve order.
_ => merge_sort(iterators.into_iter().map(Box::pin).collect()),
};

It affects the number of iterators in the futures::stream::iter(iterators) in Line 496, which is also the number of futures awaited at the same time. Single-threaded manner here is ok and we only want to make the IO concurrency.

@st1page
Copy link
Contributor Author

st1page commented Oct 11, 2023

Oh, I get it now, So futures::stream::iter().flatten() will only concat all the iterators but not join the iterators together? So we need to make it concurrent too 🤔 we can just change it to flatten_unordered?

@BugenZhao
Copy link
Member

BugenZhao commented Oct 11, 2023

Single-threaded manner here is ok and we only want to make the IO concurrency.

I'm sorry but by "single-threaded" here I mean the I/O is not concurrent as well.

@xxchan
Copy link
Member

xxchan commented Oct 13, 2023

When no key ranges predicate on the iterator, the code will create as few as possible numbers of iterator

I saw #6927, where we intentionally to create fewer iterators, so I'm a little confused what to optimize for. 🤡

@st1page
Copy link
Contributor Author

st1page commented Oct 13, 2023

When no key ranges predicate on the iterator, the code will create as few as possible numbers of iterator

I saw #6927, where we intentionally to create fewer iterators, so I'm a little confused what to optimize for. 🤡

we need to scan concurrently for IO bound scan by polling multiple iterators at one time. So we need to split the range into multiple pieces.

@st1page st1page closed this as completed Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants