Change merkledb caches to be size based #1947
Conversation
StephenButtolph
requested review from
danlaine,
darioush and
dboehm-avalabs
as code owners
x/merkledb/db.go
Outdated
| @@ -31,7 +31,7 @@ import ( | |||
| ) | |||
|
|
|||
| const ( | |||
| DefaultEvictionBatchSize = 100 | |||
| DefaultEvictionBatchSize = 256 * units.KiB | |||
There was a problem hiding this comment.
Seems like we may want bigger than this?
There was a problem hiding this comment.
This is just the default used in getStandaloneTrieView which is only used in proof verification
There was a problem hiding this comment.
That said we can make it bigger. 1 MiB?
There was a problem hiding this comment.
I was thinking 1MB is a good place to start.
There was a problem hiding this comment.
Why would we want larger than this? This isn't a cache - this size that is used to evict from the cache... As dan said - this is only used (internally) for writing during proof verification... which is writing into a memdb
There was a problem hiding this comment.
welp I already changed it to 1 MiB. In any case I don't think it matters much. I also unexported it since this is only used in this package.
There was a problem hiding this comment.
More I think about this - I think the value should be 0.
There was a problem hiding this comment.
I changed the verification cache sizes and eviction batch size.
| } | ||
|
|
||
| errs.Add(c.onEviction(key, value)) | ||
| errs.Add(c.onEviction(k, v)) |
There was a problem hiding this comment.
Do we want to exit on the first error?
There was a problem hiding this comment.
I was just keeping with the prior semantics here
Why this should be merged
Allows slightly more fine-grained cache size tuning. Specifically useful for the value caches whose entry size can very significantly.
How this works
LRU -> SizedLRU
How this was tested
updated the tests