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: make sure we use multiple threads when scanning #1705

Merged
merged 2 commits into from
Dec 13, 2023

Conversation

wjones127
Copy link
Contributor

No description provided.

Comment on lines +399 to +402
// If contiguous, continue
if indices[i + 1] == indices[i] {
continue;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes a performance bug for large vectors. If we have vectors larger than the block size (1536-dim f32 vectors are 6,144 bytes while the default block size on local fs is 4K), then without this line we make a range request for every vector we take. 😱

@wjones127 wjones127 marked this pull request as ready for review December 13, 2023 01:03
Copy link
Contributor

@westonpace westonpace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the one question

@@ -294,7 +294,12 @@ impl FragmentScanner {
let stream = futures::stream::iter(simplified_predicates.into_iter().enumerate()).map(
move |(batch_id, predicate)| {
let scanner_ref = scanner.clone();
async move { scanner_ref.read_batch(batch_id, predicate).await }
tokio::task::spawn(async move { scanner_ref.read_batch(batch_id, predicate).await })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would buffered / buffer_unordered not work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do call it below, it's just not sufficient. Each of the read_batch() tasks has some CPU-bound work, which can block IO of other tasks if they are in the same thread. By spawning, we distribute the tasks amongst threads, ensuring we get concurrency.

@wjones127 wjones127 merged commit 48a4b07 into lancedb:main Dec 13, 2023
17 checks passed
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

Successfully merging this pull request may close these issues.

2 participants