Dispute-coordinator: add LRU rw cache with write batching #5434
Conversation
Signed-off-by: Andrei Sandu <[email protected]> add logs and fix tests Signed-off-by: Andrei Sandu <[email protected]> this works Signed-off-by: Andrei Sandu <[email protected]> fix Signed-off-by: Andrei Sandu <[email protected]> buffer approval voting and fix tests Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
github-actions
bot
added
the
A3-in_progress
Signed-off-by: Andrei Sandu <[email protected]>
sandreim
added
B0-silent
Signed-off-by: Andrei Sandu <[email protected]>
| self.inner.load_recent_disputes() | ||
| let read_timer = self.metrics.time_db_read_operation("recent_disputes"); | ||
| let disputes = self.inner.load_recent_disputes()?; | ||
| drop(read_timer); |
There was a problem hiding this comment.
For a fair comparision, we should probably include the cache update?
There was a problem hiding this comment.
yeah, I missed that.
There was a problem hiding this comment.
Looking at it now - the questions is do we want to include the in-memory update time in the db read operation ? If so, I must measure the entire time spent in all of these Backend fns.
| } | ||
| } | ||
|
|
||
| let entry = entry.expect("tested above; qed"); |
There was a problem hiding this comment.
Can't we get rid of this expect, by just using pattern matching above?
There was a problem hiding this comment.
It will never panic, but yeah, to play it safe I'll use a match.
| @@ -11,6 +11,8 @@ parity-scale-codec = "3.1.2" | |||
| kvdb = "0.11.0" | |||
| thiserror = "1.0.30" | |||
| lru = "0.7.5" | |||
| lru-cache = "0.1.2" | |||
There was a problem hiding this comment.
why are we using two different crates for the same thing?
lru-cache is also in maintenance mode
There was a problem hiding this comment.
That's a good point, I switched to this while I was trying to refactor using references, but I don't recall the exact reason 🥲 . I'll drop it.
| /// The entry is in memory and was modified (needs to be persisted later). | ||
| Dirty, | ||
| /// The entry is cached and has been persisted in the DB. | ||
| Persisted, |
There was a problem hiding this comment.
how Persisted is different from Cached?
why do we use a state enum instead of keeping dirty stuff separately (ala data-oriented design)?
There was a problem hiding this comment.
Persisted is the same as Cached, plus the fact that we know we wrote it to disk at least once. I was hoping to expose the state transitions as metrics in the future if this cache is useful. So that's why I am using the extra Persisted state.
We do keep stuff separately, but that's just what we evict from the LRU cache. This leads to more code and increases the complexity, as we need to lookup multiple hashmaps. I am trying to avoid this for everything else.
There was a problem hiding this comment.
I meant the following layout instead:
State {
earliest_session: Option<SessionIndex>,
recent_disputes: Option<RecentDisputes>,
candidate_votes: LruCache<(SessionIndex, CandidateHash), CandidateVotes>,
}
OverlayCache {
cached: State,
dirty: State,
}There was a problem hiding this comment.
This would work as well, but the same applies regarding complexity. The wrapper type also enables us to observe lifetime of cache entries in the future.
| #[cfg(test)] | ||
| pub const WRITE_BACK_INTERVAL: Duration = Duration::from_secs(0); | ||
|
|
||
| /// A read-only cache (LRU) which records changes and outputs them as `BackendWriteOp` which can |
There was a problem hiding this comment.
I find the read-only part confusing
…lkadot into sandreim/overlayed_backend
Signed-off-by: Andrei Sandu <[email protected]>
#5359
More details: TBD.