Optimizes AppendVec::scan_pubkeys() when using file io #2077
Conversation
brooksprumo
force-pushed
the
append-vec/scan-pubkeys
branch
3 times, most recently
from
f9204db to
31158ff
Compare
brooksprumo
changed the title
brooksprumo
force-pushed
the
append-vec/scan-pubkeys
branch
from
31158ff to
f7b1f92
Compare
|
@HaoranYi I'm requesting a review before CI finishes because this PR has already successfully gone through CI prior to me rebasing. |
accounts-db/src/append_vec.rs
Outdated
| callback(stored_meta.pubkey()); | ||
| }); | ||
| AppendVecFileBacking::File(file) => { | ||
| let buffer_size = std::cmp::min(SCAN_BUFFER_SIZE, self.len()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Probably we can write a benchmark: generate 20K account, each account data 1k-10K data and try different buffer size?
There was a problem hiding this comment.
Also, it seems that we can reduce the size of the buffer for scan_index too.
There was a problem hiding this comment.
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.
Problem
As noted in #1394 (comment), when using file i/o to access append vecs, the
scan_pubkeys()implementation is not optimal. It currently callsscan_accounts(), which will copy all the account fields—including the data—even though only the pubkey is needed.We call
scan_pubkeys()fromcleaninside ofconstruct_candidate_clean_keys(), so having a more-optimal impl is beneficial.Summary of Changes
Only get the pubkey from each account when scanning.