[MP][Optimize] Skip locked keys during LRU eviction to improve eviction efficiency#2978
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a key filtering mechanism into the eviction process to skip locked objects. It adds an is_key_evictable method to the L1Manager and updates the eviction policy interface and LRU implementation to support an optional key_filter callable. Comprehensive tests for the new filtering logic and evictability checks are included. Feedback suggests lowering the log level from warning to debug in is_key_evictable to account for expected race conditions during concurrent deletions.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b2cbe0d. Configure here.
|
@ApostaC could you help take a look? Thanks! |
| def get_eviction_actions( | ||
| self, | ||
| expected_ratio: float, | ||
| key_filter: Callable[[ObjectKey], bool] | None = None, |
There was a problem hiding this comment.
tiny nit: maybe name with something that implies whether True or False means eligible or not e.g. key_eligible_filter or key_eligible_fn
feel free not to take, I also really like the filter naming
There was a problem hiding this comment.
@sammshen Thanks for your review, I have updated.
Signed-off-by: idellzheng <idellzheng@tencent.com>
Signed-off-by: idellzheng <idellzheng@tencent.com>
c6f753b to
132b185
Compare
…on efficiency (LMCache#2978) * [MP][optimize] optimize lru eviction Signed-off-by: idellzheng <idellzheng@tencent.com> * add ut Signed-off-by: idellzheng <idellzheng@tencent.com> * delete log Signed-off-by: idellzheng <idellzheng@tencent.com> * rename Signed-off-by: idellzheng <idellzheng@tencent.com> --------- Signed-off-by: idellzheng <idellzheng@tencent.com>
…on efficiency (LMCache#2978) * [MP][optimize] optimize lru eviction Signed-off-by: idellzheng <idellzheng@tencent.com> * add ut Signed-off-by: idellzheng <idellzheng@tencent.com> * delete log Signed-off-by: idellzheng <idellzheng@tencent.com> * rename Signed-off-by: idellzheng <idellzheng@tencent.com> --------- Signed-off-by: idellzheng <idellzheng@tencent.com>
…on efficiency (LMCache#2978) * [MP][optimize] optimize lru eviction Signed-off-by: idellzheng <idellzheng@tencent.com> * add ut Signed-off-by: idellzheng <idellzheng@tencent.com> * delete log Signed-off-by: idellzheng <idellzheng@tencent.com> * rename Signed-off-by: idellzheng <idellzheng@tencent.com> --------- Signed-off-by: idellzheng <idellzheng@tencent.com>
In the current LRU eviction implementation, get_eviction_actions blindly selects the first target_count keys from the LRU order without checking whether those keys are actually evictable (i.e., not locked).
For example, if target_count = 10 but 8 out of the first 10 LRU keys are currently read-locked or write-locked, the subsequent delete call can only successfully remove 2 keys. Meanwhile, there may be plenty of unlocked keys further down the LRU order that could have been evicted instead, this leads to extremely low eviction efficiency and prevents memory from being released in a timely manner.
Note
Medium Risk
Changes eviction selection logic and the
EvictionPolicy.get_eviction_actionsinterface, which could affect eviction behavior and call sites; however the change is localized and covered by new unit tests.Overview
Improves L1 eviction efficiency by skipping read/write-locked keys when choosing LRU eviction candidates.
This extends
EvictionPolicy.get_eviction_actionswith an optionalkey_eligible_filtercallback, implements filtering inLRUEvictionPolicy, and wiresL1EvictionControllerto passL1Manager.is_key_evictableso eviction avoids non-deletable keys.Adds unit tests for
L1Manager.is_key_evictableand for the new LRU filter behavior (including order/ratio handling and all/none acceptance cases).Reviewed by Cursor Bugbot for commit 132b185. Bugbot is set up for automated code reviews on this repo. Configure here.