Evicts from the accounts read cache in the background #575
Conversation
brooksprumo
force-pushed
the
evictor/background
branch
from
f25fc1c to
26d49b8
Compare
brooksprumo
force-pushed
the
evictor/background
branch
from
fae89f6 to
456d484
Compare
brooksprumo
force-pushed
the
evictor/background
branch
from
456d484 to
f01470d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #575 +/- ##
========================================
Coverage 81.8% 81.9%
========================================
Files 851 851
Lines 230165 230291 +126
========================================
+ Hits 188464 188622 +158
+ Misses 41701 41669 -32 |
jeffwashington
left a comment
jeffwashington
left a comment
There was a problem hiding this comment.
lgtm
Appears to improve perf for critical path of tx processing.
|
|
||
| /// Channel to send eviction requests | ||
| /// | ||
| /// NOTE: This field must be above `evictor` to ensure it is dropped before `evictor`. |
There was a problem hiding this comment.
The comment seems confusing. I thought the fields are destructed backward?
_evictor drop first then evict_sender?
There was a problem hiding this comment.
Rust drops fields top to bottom as declared in the struct.
From https://doc.rust-lang.org/reference/destructors.html:
The fields of a struct are dropped in declaration order.
There was a problem hiding this comment.
oh. interesting. That's different from C++, which destruct in reverse order.
| // when we loop around we'll find a message that we should evict again. | ||
| // However the current data size likely is not higher than the high water mark. | ||
| // So, check the current size to see if this was a spurious wakeup. | ||
| if data_size.load(Ordering::Relaxed) <= max_data_size_hi { |
| queue: &Mutex<IndexList<ReadOnlyCacheKey>>, | ||
| ) -> u64 { | ||
| let mut num_evicts = 0; | ||
| while data_size.load(Ordering::Relaxed) > target_data_size { |
|
I think we can move the whole store to background. I will put up a pr to experiment with this idea. |
4 participants
Problem
Evicting from the accounts read cache is done in the foreground. This can cause transaction processing to stall while this eviction occurs.
Summary of Changes
Evict in the background.
Note, the goal of this PR is to move evictions to the background. I've purposely left how we evict the same. It is likely that we can further optimize the eviction logic.
Here's some results! I set up two dev boxes against mnb. One running master and one running this PR. The charts are over a 12 hour period, so a good chunk of time.
Results:
storeDuring transaction processing, we need to load accounts. If the account is not already in the write cache, we check the read cache. If in neither, we need to load from disk. If the account is loaded as read-only, we'll store it into the read cache.
Right now that
storeis then in the critical path for transaction processing. Currently, at the end ofstore, we alsoevict. So by movingevictto the background, do we improve anything?Here's how long we spend
storeing:Top = sum
Bottom = mean
Lower = better
Purple = master
Blue = this pr
This PR improves
storetime.Results:
evictSince eviction has been moved to the background, lets look at how long it takes to evict:
Top = sum
Bottom = mean
Lower = better
Purple = master
Blue = this pr
This PR slightly improves
evicttime. It's not a ton, as we still are evicting about the same number of items, and about as often. But we are no longer trying to evict from multiple threads concurrently, which is likely a win.Another way to explain evict time is that fewer items are evicted with this PR. Here's the count of evictions:
Results:
loadDoes evicting in the background impact loading from the read cache? Do we increase/decrease contention on any of the internal data structures? Here's the load time:
Top = sum
Bottom = mean
Lower = better
Purple = master
Blue = this pr
We see that loading is a little faster with this PR too. Maybe another function of reduced contention?
Results: replay time, loading transaction accounts
And now where it matters most: does the PR help loading transaction accounts?
Top = sum
Bottom = mean
Lower = better
Purple = master
Blue = this pr
Metrics indicate there's a slight improvement when loading accounts too.
Results: loading transaction accounts histogram
Here's a histogram for how long it takes to load accounts. At the p10, p25, p50, and p75, load times are very similar between master and this PR. Once we start getting higher, this PR does load accounts faster. Most notable are three-9s, four-9s, and five-9s:
This is on a log scale, and there are three groups of two. Lower is better. Each lower line in the group is this PR, and the higher line is master. Here we can see more effects of how background evicting improves these worst-case load times.