Extend lifecycle trait with on_evict_(cold|hot)#117
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Adds on_evict_hot and on_evict_cold hooks to the Lifecycle trait so callers can distinguish which queue an evicted entry came from (resolves #39). The existing on_evict becomes a deprecated, provided method to preserve source-level backwards compatibility, and shard.rs call sites are updated to dispatch to the appropriate new hook.
Changes:
- Add
on_evict_hot/on_evict_coldprovided methods onLifecycle, withon_evictnow#[deprecated]and given an empty default body that the new methods delegate to. - Update eviction call sites in
src/shard.rs(cold ring, hot ring, replacement ininsert_existing, placeholder rejection, and oversized-insert handlers) to call the new methods.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/lib.rs | Adds new trait methods, deprecates on_evict, and changes it from required to provided. |
| src/shard.rs | Replaces on_evict calls with on_evict_hot / on_evict_cold; uses enter_state to dispatch in insert_existing, unconditionally on_evict_hot in overweight/placeholder paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
1132
to
1135
| if let Some((idx, _)) = self.search_resident(hash, &key) { | ||
| if let Some((ek, ev)) = self.remove_internal(hash, idx) { | ||
| self.lifecycle.on_evict(lcs, ek, ev); | ||
| self.lifecycle.on_evict_hot(lcs, ek, ev); | ||
| } |
Comment on lines
+1064
to
+1140
| @@ -1122,13 +1131,13 @@ impl< | |||
| // Make sure to remove any existing entry | |||
| if let Some((idx, _)) = self.search_resident(hash, &key) { | |||
| if let Some((ek, ev)) = self.remove_internal(hash, idx) { | |||
| self.lifecycle.on_evict(lcs, ek, ev); | |||
| self.lifecycle.on_evict_hot(lcs, ek, ev); | |||
| } | |||
| } | |||
| if matches!(strategy, InsertStrategy::Replace { .. }) { | |||
| return Err((key, value)); | |||
| } | |||
| self.lifecycle.on_evict(lcs, key, value); | |||
| self.lifecycle.on_evict_hot(lcs, key, value); | |||
| fn on_evict(&self, state: &mut Self::RequestState, key: Key, val: Val); | ||
| #[deprecated( | ||
| since = "0.6.22", | ||
| note = "Use `on_evict_hot` or `on_evict_cold` instead, depending on the desired semantics. This method will still be called by default to preserve backwards compatibility, but it won't be called if either of the new methods are implemented." |
Comment on lines
+199
to
+204
| #[deprecated( | ||
| since = "0.6.22", | ||
| note = "Use `on_evict_hot` or `on_evict_cold` instead, depending on the desired semantics. This method will still be called by default to preserve backwards compatibility, but it won't be called if either of the new methods are implemented." | ||
| )] | ||
| #[allow(unused_variables)] | ||
| fn on_evict(&self, state: &mut Self::RequestState, key: Key, val: Val) {} |
Comment on lines
+199
to
+218
| #[deprecated( | ||
| since = "0.6.22", | ||
| note = "Use `on_evict_hot` or `on_evict_cold` instead, depending on the desired semantics. This method will still be called by default to preserve backwards compatibility, but it won't be called if either of the new methods are implemented." | ||
| )] | ||
| #[allow(unused_variables)] | ||
| fn on_evict(&self, state: &mut Self::RequestState, key: Key, val: Val) {} | ||
|
|
||
| /// Called when an item is evicted from the cold queue. | ||
| #[inline] | ||
| fn on_evict_cold(&self, state: &mut Self::RequestState, key: Key, val: Val) { | ||
| #[allow(deprecated)] | ||
| self.on_evict(state, key, val) | ||
| } | ||
|
|
||
| /// Called when an item is evicted from the hot queue. | ||
| #[inline] | ||
| fn on_evict_hot(&self, state: &mut Self::RequestState, key: Key, val: Val) { | ||
| #[allow(deprecated)] | ||
| self.on_evict(state, key, val) | ||
| } |
- Fix hot/cold misclassification in handle_insert_overweight: capture the evicted entry's ResidentState before remove_internal so cold evictions aren't reported as hot. - Reword on_evict deprecation note to accurately describe per-method fallback (each new method delegates to on_evict independently). - Document rejection-style behavior on on_evict_hot: items that never enter the cache (oversized inserts and oversized placeholder values) are reported as hot. - Migrate DefaultLifecycle (sync and unsync) and the eviction_listener example to implement on_evict_hot/on_evict_cold directly.
Owner
|
Thanks @DenizUgur! I pushed a commit that addresses the Copilot review. Disposition per comment:
|
Contributor
Author
|
So to make sure I understand correctly, if I were to use UnitWeighter and the queue is full do the new items reported as hot evictions? |
…cold - on_evict stays as a provided method with an empty default body (no deprecation). on_evict_hot/on_evict_cold default to delegating to it, so existing impls keep working and new impls can override just the hot/cold methods when they want to distinguish. - Route oversized inserts and oversized placeholder rejections through on_evict_cold (they never entered any queue). - Revert DefaultLifecycle (sync/unsync) and the eviction_listener example to the simple on_evict form.
Owner
|
It depends on the state of the cache, you can get both cold and hot evictions regardless of Weighter. |
Comment on lines
+203
to
+208
| /// | ||
| /// If none of `on_evict`, `on_evict_hot`, or `on_evict_cold` is overridden, | ||
| /// eviction notifications are silently dropped. | ||
| #[allow(unused_variables)] | ||
| #[inline] | ||
| fn on_evict(&self, state: &mut Self::RequestState, key: Key, val: Val) {} |
| self.entries.remove(placeholder.idx()); | ||
| self.map_remove(placeholder.hash(), placeholder.idx()); | ||
| self.lifecycle.on_evict(lcs, key, value); | ||
| self.lifecycle.on_evict_cold(lcs, key, value); |
Comment on lines
+222
to
+228
| /// Called when an item is evicted from the hot queue. | ||
| /// | ||
| /// By default delegates to [`Lifecycle::on_evict`]. | ||
| #[inline] | ||
| fn on_evict_hot(&self, state: &mut Self::RequestState, key: Key, val: Val) { | ||
| self.on_evict(state, key, val) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added new
on_evict_coldandon_evict_hotmethods toLifecycletrait to be able to distinguish where the resident is being evicted from. This is useful for tracking the age of the evicted residents.fixes #39