Fix LRU cache eviction behavior #2353
Conversation
wxing1292
requested review from
dynajoe,
samarabbas,
meiliang86,
yiminc and
a team
* Before this PR, LRU cache will only attempt to evict the last access cache item, if this item is pinned, cache is considered full. * After this PR, LRU cache will attempt to evict the last access & evictable cache item, even if this item is in the middle. * Update unit test
| return | ||
| } | ||
|
|
||
| // entry.refCount > 0 |
There was a problem hiding this comment.
I noticed there's no guard in Release to prevent decrementing refCount to less than zero: https://github.com/temporalio/temporal/pull/2353/files#diff-2d61d82d9a8acd1031ca31021b40eb5f8766840ac54b4113dba6b50e8e033afeR241
albeit unlikely - a bug could cause the refCount to be less than zero. In which case this assertion would not be correct (refCount > 0).
There was a problem hiding this comment.
the ref count must be maintained correctly, by the caller (meaning if double release ever happen, the behavior is undefined)
the reference count > 0 means whether the cache item is being used, e.g. cache cannot evict this item if ref count > 0
the reason to use a refcount here is to guarantee all mutation to a mutable state must be serialized
- caller fist get the mutable state cache entry
- caller then use the mutable state (from the cache entry) by first locking it
about double release, there are few things that can be done
- if ref count < 0, panic (this should be long term solution, not suitable for patch)
- do <= 0 check instead of == 0 check, this will prevent some weird behavior, but when refcount < 0, system is already broken.
There was a problem hiding this comment.
synced offline, this PR is ok for patching
* Before this PR, LRU cache will only attempt to evict the last access cache item, if this item is pinned, cache is considered full. * After this PR, LRU cache will attempt to evict the last access & evictable cache item, even if this item is in the middle. * Update unit test
What changed?
Why?
Fix cache eviction behavior, so cache can be better utilized.
How did you test it?
New tests
Potential risks
N/A
Is hotfix candidate?
Yes