perf: Clear cacheKV RAM if its too large #15767
Conversation
github-prbot
requested review from
a team and
mark-rushakoff
and removed request for
a team
store/cachekv/store.go
Outdated
| } | ||
| } | ||
|
|
||
| sort.Strings(keys) | ||
| store.deleteCaches() |
There was a problem hiding this comment.
I think after the Write call, the whole cache store is discarded usually?
There was a problem hiding this comment.
Oh interesting. If so:
- This is still a win, for getting the data discarded earlier. (rather than being held with the underlying DB's writes as well)
- We should just always clear everything, rather than the old capacity preservation code then
| keys := make([]string, 0, len(store.cache)) | ||
| // Not the best. To reduce RAM pressure, we copy the values as well | ||
| // and clear out the old caches right after the copy. | ||
| sortedCache := make([]cEntry, 0, len(store.cache)) |
There was a problem hiding this comment.
Just an observation -- I assume this slice is often oversized, as it ideally match the number of dirty entries in the store.cache. That may be worth tuning in a separate PR.
There was a problem hiding this comment.
In general, the entire CacheKV store should just be rewritten entirely. We perenially run into performance issues due to some wack part of its design.
Its very overly complicated, for little reason
store/cachekv/store.go
Outdated
| } | ||
| } | ||
|
|
||
| sort.Strings(keys) | ||
| store.deleteCaches() |
There was a problem hiding this comment.
I think this method would read easier if resetting the caches happened before assigning into sortedCache, even if you need to store len(store.cache) first.
There was a problem hiding this comment.
I don't understand, you can only reset the caches, once you've grabbed a copy of the data?
|
Friendly bump on this PR |
|
@kocubinski could we get your review in this? |
There was a problem hiding this comment.
LGTM @ValarDragon. I would say the arbitrary 100k value would warrant a benchmark though.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for key := range store.cache { | ||
| delete(store.cache, key) | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map
| for key := range store.unsortedCache { | ||
| delete(store.unsortedCache, key) | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map
| @@ -93,6 +93,28 @@ func (store *Store) Delete(key []byte) { | |||
| store.setCacheValue(key, nil, true) | |||
| } | |||
|
|
|||
| func (store *Store) resetCaches() { | |||
| if len(store.cache) > 100_000 { | |||
There was a problem hiding this comment.
In light of #15767 (comment) consider making this branch unconditional, that is, never use the cache clearing idiom.
The clearing was introduced as part of dbb9923, so I wonder how much performance is gained by the clearing itself. If not much, that's another reason to stop clearing.
| @@ -103,44 +125,40 @@ func (store *Store) Write() { | |||
| return | |||
| } | |||
|
|
|||
| type cEntry struct { | |||
| key string | |||
| val *cValue | |||
There was a problem hiding this comment.
How about
type cEntry struct {
key string
value []byte
}
to shave off a field from each cache key? val.dirty is never referenced.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Description
Clear the CacheKV store's RAM if its too large. If the CacheKV's caches are too large, we've likely done some linear time workload.
The old design of never clearing this memory, meant significant RAM increases (all genesis size) throughout the entire InitGenesis process, unclear to me if this RAM overhead persisted in subsequent block processing. (Sometimes 2x large portions of genesis state, if the module does iteration and has a sorted cache) Most other RAM overheads throughout init genesis, are at least module-local for extra data.
This was 20+ GB reduced in peak Osmosis RAM usage during InitGenesis from a state export. This significantly reduces the RAM pressure.
This should also come in handy for Osmosis epoch blocks.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change