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.

Sign up for GitHub

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

Optimizes AppendVec::scan_pubkeys() when using file io #2077

Merged
merged 3 commits into from
Jul 11, 2024
Merged

Optimizes AppendVec::scan_pubkeys() when using file io #2077

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f7b1f92
Optimizes AppendVec::scan_pubkeys() when using file io
brooksprumo Jul 9, 2024
202d916
pr: use PAGE_SIZE for the buffer size instead of SCAN_BUFFER_SIZE
brooksprumo Jul 10, 2024
0dae0f1
set scan buffer size for scan_pubkeys()
brooksprumo Jul 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions accounts-db/src/append_vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1034,10 +1034,18 @@ impl AppendVec {
offset = next.next_account_offset;
}
}
AppendVecFileBacking::File(_file) => {
self.scan_accounts(|stored_meta| {
callback(stored_meta.pubkey());
});
AppendVecFileBacking::File(file) => {
let buffer_size = std::cmp::min(SCAN_BUFFER_SIZE, self.len());
Copy link

@HaoranYi HaoranYi Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

SCAN_BUFFER_SIZE is around 30MB. And the data, which we are really interested in, is 136B. It seems like we are over fetching the buffer and don't skip much of the account's data.

For example, let's say that account data.len is about 100K. Then, each time we read, we fetch up to 300 accounts in the buffer. And we will only skip 1 account after we used all 300 accounts in the buffer. then fetch another 300 Accounts... Only 1 out of 300 accounts are skipped.

How about making 'SCAN_BUFFER_SIZE' smaller, i.e. 1K?
For the example above, 1K buffer will nicely skip loading 99K data of the accounts.

Copy link

@HaoranYi HaoranYi Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe 1M is better.
There is a tradeoff to consider. On one hand, it would be more efficient to read larger buffer at a time. On the other hand, too large a buffer would cause us to read too much unused data...

Copy link

Choose a reason for hiding this comment

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

wdyt?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I think this is a valid concern. And yes, I agree, it is a tradeoff. The point of this PR is to avoid unnecessary reading/copying, so a smaller buffer is better in that regard. However, we also don't want a lot of syscalls; a larger buffer is better in that case. I'm not sure what the sweet spot is. IOW, at what buffer size is it equivalently expensive to make a syscall?

I've bumped it down to a page size. Maybe that's too small?

Copy link

Choose a reason for hiding this comment

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

Probably we can write a benchmark: generate 20K account, each account data 1k-10K data and try different buffer size?

Copy link

Choose a reason for hiding this comment

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

Also, it seems that we can reduce the size of the buffer for scan_index too.

Copy link
Author

Choose a reason for hiding this comment

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

Good idea; I'm working on a benchmark now.

With preliminary benchmark results, I've found a buffer size of 256 KiB to work well. I think this should be good enough for now, for this PR. File IO is only used by us for testing, and I'd like to get some value in here so we can test on mnb too.

Copy link

Choose a reason for hiding this comment

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

ok. sounds good to me.

let mut reader =
BufferedReader::new(buffer_size, self.len(), file, STORE_META_OVERHEAD);
while reader.read().ok() == Some(BufferedReaderStatus::Success) {
let (_offset, bytes) = reader.get_offset_and_data();
let (stored_meta, _next) = Self::get_type::<StoredMeta>(bytes, 0).unwrap();
callback(&stored_meta.pubkey);
// since we only needed to read the pubkey, skip ahead to the next account
let stored_size = aligned_stored_size(stored_meta.data_len as usize);
reader.advance_offset(stored_size);
}
}
}
}
Expand Down