Perf: Relax locking contention for cache and cachekv#353
Conversation
yzang2019
requested review from
codchen,
philipsu522,
stevenlanders and
udpatil
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #353 +/- ##
==========================================
- Coverage 55.41% 55.38% -0.04%
==========================================
Files 620 620
Lines 51694 51605 -89
==========================================
- Hits 28646 28581 -65
+ Misses 20964 20941 -23
+ Partials 2084 2083 -1
|
stevenlanders
left a comment
stevenlanders
left a comment
There was a problem hiding this comment.
This looks good - it might make sense to do a gobench test before/after the change to help double-check the throughput increases (hard to eyeball, this could be much faster, not sure)
store/cachekv/store.go
Outdated
| return cv.(*types.CValue).Value() | ||
| } else { | ||
| value = cacheValue.Value() | ||
| return store.parent.Get(key) |
There was a problem hiding this comment.
small nit, we can drop the else block here and just return
There was a problem hiding this comment.
True, will be fix
| // We need a copy of all of the keys. | ||
| // Not the best, but probably not a bottleneck depending. | ||
| keys := make([]string, 0, store.cache.Len()) | ||
| keys := []string{} |
There was a problem hiding this comment.
To avoid the allocations, keeping the size at 0 and capacity at store.cache.Len() can actually be better.
There was a problem hiding this comment.
Yeah, but unfortunately, sync map doesnt support length, that's why we remove all length here
| // and not allocating fresh objects. | ||
| // Please see https://bencher.orijtech.com/perfclinic/mapclearing/ | ||
| store.cache.DeleteAll() | ||
| store.cache.Range(func(key, value any) bool { |
There was a problem hiding this comment.
side node: I wonder if it would make sense to just set the cache to a new map (requires concurrency protection) and let the garbage collector clean up the old one. This is logically fine.
There was a problem hiding this comment.
Yeah this is good questions, I've thought about that, but I think it could be risky, I'm not sure why initially we are not doing that tbh, so better to just keep the same logic first.
## Describe your changes and provide context This PR reverts the change in sei-protocol/sei-cosmos#353 and sei-protocol/sei-cosmos#391 until we have OCC fully enabled. ## Testing performed to validate your change Unit test coverage
## Describe your changes and provide context This PR reverts the change in sei-protocol/sei-cosmos#353 and sei-protocol/sei-cosmos#391 until we have OCC fully enabled. ## Testing performed to validate your change Unit test coverage
3 participants
Describe your changes and provide context
Problem:
Currently when doing profiling, there are lot of locking contention happening in the cachekv layer, this is because we are using mutex for all read and write keys, but cachekv as a transient cache doesn't really need such a strict locking mechanism. Having a high locking contention would hurt the parallelize transaction execution performance a lot.
Solution:
Testing performed to validate your change
Fully tested in loadtest env