Change read-only-cache to be keyed on pubkey only

[HaoranYi]

Problem

The way that readonly cache is keyed on (pubkey, slot) can lead to caching
stale entries and inefficient use of the cache.

By design, readonly cache is shadowed by write_cache for the fork tips. Because
of this, it is very unlikely that we need more than one entry per pubkey in the
cache. Extra entry in the cache is a waste since it is from stale slot and will
eventually be evicted over the time.

Also, the extra slot in the key increase the FIFO size for LRU.

Summary of Changes

Rework readonly cache to be keyed on pubkey only.

Fixes #

[HaoranYi]

read_only_accounts_cache_entries=31822i read_only_accounts_cache_uniq_keys=30890i read_only_accounts_cache_max_dups=2i
read_only_accounts_cache_entries=31709i read_only_accounts_cache_uniq_keys=30820i read_only_accounts_cache_max_dups=2i
read_only_accounts_cache_entries=30828i read_only_accounts_cache_uniq_keys=30052i read_only_accounts_cache_max_dups=2i
read_only_accounts_cache_entries=30417i read_only_accounts_cache_uniq_keys=29720i read_only_accounts_cache_max_dups=2i

[HaoranYi]

[jeffwashington]

i don't understand the 4 logs lines. What is the important data and difference?

[jeffwashington]

I'm concerned about getting the wrong data with various race conditions I could imagine. using the slot enforces that we are getting the right version of each account.
maybe: read account, goes into write cache
write account, wiped from write cache
other fork reads and puts account into write cache
flush main fork's write cache in bg. after index entry gets updated to newly flushed slot, entry is already in write cache from other fork at a different (older, newer, different fork?) slot than the one that just got flushed. So, that wrong one gets loaded.

We don't clear the read cache when we drop slots or forks. That's an issue for this scheme, I think. The index is updated correctly in those cases (I have to assume).

[HaoranYi]

I'm concerned about getting the wrong data with various race conditions I could imagine. using the slot enforces that we are getting the right version of each account.

In this PR, we are still checking the slot. The slot is stored along the 'AccountSharedData' in the value. When loading, we check both pubkey (from the map) and slot (from the value).

[HaoranYi]

i don't understand the 4 logs lines. What is the important data and difference?

read_only_accounts_cache_entries records the number of entries in the cache, i.e. distinct (pubkey, slot) pairs.
read_only_accounts_cache_uniq_keys records the number of unique pubkeys in the cache.
read_only_accounts_cache_max_dups records the max number of slots per pubkey in the cache.

Take read_only_accounts_cache_entries=31709i read_only_accounts_cache_uniq_keys=30820i read_only_accounts_cache_max_dups=2i as an example. The cache stored 31,709 (pubkey, slot). Only have 30,820 unique pubkeys. Therefore, given that max_num_slot per pubkey is 2, there are 889 pubkeys that have two slots of data stored in the cache, one of these two slots would be stale data. Percentage wise, it is around 3%.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 91.66667% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 81.9%. Comparing base (c0be86d) to head (21952ae).
Report is 28 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #576   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         851      851           
  Lines      230475   230514   +39     
=======================================
+ Hits       188785   188818   +33     
- Misses      41690    41696    +6     

[HaoranYi]

Rebase to master to pick up brook's background evicts changes.

• replay load time
  

• hit/miss/evicts
  

• another view of hit/miss (log)
  

In all the graph, 4wp represents this pr. yh14 represents master.

[jeffwashington]

so in theory, writing an account would evict the pubkey from the read cache completely. The theory must be that there are accounts that are flushed to append vecs, read to put in read cache, then written 100+ slots later, then flushed, then read from the newer slot and put in the read cache where now the account is in the read cache twice (or more)?
@HaoranYi can you run mnb for a bit and run some debugging code that tells us how many accounts are in the read only cache multiple times and how big the accounts that are in there multiple times are?
would be great to know this pattern.

[HaoranYi]

so in theory, writing an account would evict the pubkey from the read cache completely. The theory must be that there are accounts that are flushed to append vecs, read to put in read cache, then written 100+ slots later, then flushed, then read from the newer slot and put in the read cache where now the account is in the read cache twice (or more)? @HaoranYi can you run mnb for a bit and run some debugging code that tells us how many accounts are in the read only cache multiple times and how big the accounts that are in there multiple times are? would be great to know this pattern.

Sure. I will run this experiment.

[brooksprumo]

so in theory, writing an account would evict the pubkey from the read cache completely. The theory must be that there are accounts that are flushed to append vecs, read to put in read cache, then written 100+ slots later, then flushed, then read from the newer slot and put in the read cache where now the account is in the read cache twice (or more)?

Why doesn't this "written 100+ slots later" cause the account be removed from the read cache though?

Is it because when we store to the write cache and remove from the read cache, we use the slot being stored to as a check into the read cache? Presumably this check in the read cache would always fail, and thus never remove old read cache entries. Maybe that's the crux of the change—that we can now correctly remove entries in the read cache that have been superseded by entries in the write cache?

[HaoranYi]

so in theory, writing an account would evict the pubkey from the read cache completely. The theory must be that there are accounts that are flushed to append vecs, read to put in read cache, then written 100+ slots later, then flushed, then read from the newer slot and put in the read cache where now the account is in the read cache twice (or more)?

Why doesn't this "written 100+ slots later" cause the account be removed from the read cache though?

Is it because when we store to the write cache and remove from the read cache, we use the slot being stored to as a check into the read cache? Presumably this check in the read cache would always fail, and thus never remove old read cache entries. Maybe that's the crux of the change—that we can now correctly remove entries in the read cache that have been superseded by entries in the write cache?

Yes. Writing account at slot 'x' won't remove cached entry of slot 'y'. So stale cache entry at slot 'y' will only be removed until it becomes LRU.
Also having (pubkey, slot) as the cache key, it means that we need to keep both (pubkey, slot) in the LRU list too, which also adds 8 bytes of unnecessary storage cost per element.

The single pubkey ensures that we have at most one entry per pubkey. And it also helps reduce the numbers of evicts. The stale element from the same pubkey will be replaced when new updates are inserted, which saves the cost of evictions in the current code base (as shown in the 2nd graph on the right). Of course, evicts matter less now thanks to the background evicts 😉

[brooksprumo]

I like the idea of this change!

Should there be new tests added that ensure

1. storing in a new slot "removes" the old one?
2. calling remove on a pubkey works regardless of its slot?
3. calling load correctly checks slot?

Some (all?) of these may be covered by existing tests too.

[HaoranYi]

Another aspect of this change is to help when we scale the cache size.
With current code, no matter how much cache size we allocated, the cache will always be full (i.e. account_size pegged at max cache size) because stale data won't be evicted and keep staying in the cache until the cache is full and then it will be LRUs.

[HaoranYi]

3. calling load correctly checks slot?
   Added coverage.

[HaoranYi]

1. storing in a new slot "removes" the old one?
   Added coverage.

[HaoranYi]

1. storing in a new slot "removes" the old one?

Added.

2. calling remove on a pubkey works regardless of its slot?

Already covered
solana/accounts-db/src/read_only_accounts_cache.rs:468

3. calling load correctly checks slot?

Added.

[brooksprumo]

Lgtm! Makes the read cache more efficient, and thus improves the time to load accounts.

[HaoranYi]

Got @jeffwashington's approval from slack.