LRU Cache for Boxes/KVs#4275
Conversation
| return | ||
| } | ||
|
|
||
| type persistedValue struct { |
There was a problem hiding this comment.
I just renamed and moved this but I don't mind undoing it if there's an objection.
There was a problem hiding this comment.
@algoidurovic I think the rename touches on a central question: What is box-specific about the cache?
I realize the question prompts a potentially large renaming exercise, but I'm wondering if the cache ought to be kv themed rather than box themed.
There was a problem hiding this comment.
yes, I think it should. Anything below the ledger tries to talk about kv rather than about boxes. But I don't think it changes much code.
There was a problem hiding this comment.
@algoidurovic From my perspective, based on renames to KV, the thread is now resolved. Since I didn't open it, I'm leaving it open for you to resolve.
|
|
||
| persistedData, err := au.accountsq.lookupKeyValue(key) | ||
| if persistedData.round == currentDbRound { | ||
| return persistedData.value, nil |
There was a problem hiding this comment.
Error handling has been slightly changed here, but seems innocent enough to me.
ahangsu
left a comment
ahangsu
left a comment
There was a problem hiding this comment.
just some random questions, the implementation looks following previous implementation patterns, should be legit
Codecov Report
@@ Coverage Diff @@
## feature/avm-box #4275 +/- ##
===================================================
+ Coverage 55.33% 55.35% +0.01%
===================================================
Files 403 405 +2
Lines 51406 51505 +99
===================================================
+ Hits 28448 28510 +62
- Misses 20529 20558 +29
- Partials 2429 2437 +8
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
jannotti
left a comment
jannotti
left a comment
There was a problem hiding this comment.
I'm mostly through, need to review the lruboxes.go file by diffing it with the other caches.
But I want to repeat my agreement with Michael. All these things should use "key value" language, not box language. At this level, they are key/value pairs, and it so happens they only have box data in them. But the lookup is for a key/value pair, not a box.
|
|
||
| // baseBoxesPendingBufferSize defines the size of the base boxes pending buffer size. | ||
| // At the beginning of a new round, the entries from this buffer are being flushed into the base boxes map. | ||
| const baseBoxesPendingBufferSize = 5000 |
There was a problem hiding this comment.
@algoidurovic Can the inline comment be expanded (or a new one added) to summarize why we chose the configured value?
I realize the value is liable to change prior to merge. Consider the note a visual reminder for when we're ready to finalize the value.
michaeldiamant
left a comment
michaeldiamant
left a comment
There was a problem hiding this comment.
@algoidurovic Thanks for your effort to implement + test a KV-specific cache implementation!
Approving with the understanding that we'll finalize configuration of baseKVPendingBufferSize out-of-band to the PR.
jannotti
left a comment
jannotti
left a comment
There was a problem hiding this comment.
Some feedback, I'm going to go pull this down locally and diff the lru now, since github review isn't good for that.
|
|
||
| // before compares the round numbers of two persistedKVData and determines if the current persistedKVData | ||
| // happened before the other. | ||
| func (prd *persistedKVData) before(other *persistedKVData) bool { |
There was a problem hiding this comment.
Since they are small objects, I might not use pointers for the receiver or the arg.
| // baseResources stores the most recently used resources, at exactly dbRound | ||
| baseResources lruResources | ||
|
|
||
| // baseKV stores the most recently used KV, at exactly dbRound |
There was a problem hiding this comment.
I'd call it either baseKVs or baseKVStore, as it is a bunch of Key/Values. (right?)
| persistedData, err := au.accountsq.lookupKeyValue(key) | ||
| if persistedData.round == currentDbRound { | ||
| return persistedData.value, nil | ||
| if err == nil { |
There was a problem hiding this comment.
Pavel recently made a PR to change the error handling on the similar functions that would be good to copy here.
| persistedData, err = au.accountsq.lookupResources(addr, aidx, ctype) | ||
| if persistedData.round == currentDbRound { | ||
| if persistedData.addrid != 0 { | ||
| if err == nil { |
There was a problem hiding this comment.
Why is this PR changing lookupResource?
There was a problem hiding this comment.
Good catch, I don't know how that got in there.
| // where persistedData.value == nil to avoid unnecessary db lookups | ||
| // for deleted KVs. | ||
| au.baseKVs.writePending(persistedData, key) | ||
| return persistedData.value, err |
There was a problem hiding this comment.
Now that err is checked earlier, let's make this explicitly nil so it is obvious it's the good path.
There was a problem hiding this comment.
got it, fixed.
4 participants
Summary
This KV cache is intended to nearly exactly replicate the resources cache, in lieu of generics. Included are also the box database benchmark tests that were the original motivation for implementing a cache. One of the size limits of the cache (
baseKVPendingAccountsBufferSize = 50,000) is informed by the maximum box size (~32KB) and thebaseResourcesPendingAccountsBufferSize = 100,000where the maximum resource size is ~16KB. The other limitation on cache size islen(au.kvStore): the number of KV modifications in the state deltas. While the theoretical maximum memory usage is quite high, this is an existing problem with the current cache implementations and should be addressed in another PR.Test Plan
The cache and underlying data structures are tested in unit tests.