Adding OnEvict callback for when items are removed #28
Conversation
One or more callbacks can be registered to fire when items are removed from the cache. This is done synchronously to and Delete, Set, or MSet calls, as well as when the reaper thread finds expired items to remove. To keep the cost low, the count of registered callbacks is checked before any action is taken. For existing LazyLRU users, this will add the cost of one `atomic.LoadInt32` to these operations. As compared to the locks that already exist, this is negligible.
lazylru.go
Outdated
| if len(deathList) == 0 { | ||
| return | ||
| } | ||
| if atomic.LoadInt32(&(lru.numEvictCB)) == 0 { |
There was a problem hiding this comment.
nit: Any reason to do this over just storing an atomic.Int32 to begin with? Should be same outcome / sizing on the struct, just with better ergonomic/semantics when used.
There was a problem hiding this comment.
There is a reason, but it's a little silly. When callbacks are registered, we have to take a lock to safely add to the slice. This value is just a reflection of the length of that slice, so it is incremented at the same time. Since I'm just incrementing the value, I don't use the atomic operators to do it.
As a practical matter, I shouldn't care because the number of times that the OnEvict registration function is called should be very small and should likely occur before any items are added (let alone removed) from the LRU.
Given that there is more chance that I or some future developer forgets to read atomically from the value if it is an int32 rather than atomic.Int32 and that the number of times OnEvict is called will be 0 and occasionally 1, and the fact that OnEvict is almost guaranteed to be called in a non-contentious situation anyway so it really doesn't matter, I'm going to switch to atomic.Int32.
lazylru.go
Outdated
| @@ -189,6 +240,9 @@ func (lru *LazyLRU[K, V]) reap(start int, deathList []*item[K, V]) { | |||
| lru.lock.Unlock() | |||
| } | |||
| atomic.AddUint32(&lru.stats.ReaperCycles, cycles) | |||
| if len(aggDeathList) > 0 && atomic.LoadInt32(&lru.numEvictCB) > 0 { | |||
There was a problem hiding this comment.
nit: Likewise with earlier comment, this becomes:
if len(aggDeathList) > 0 && lru.numEvictCB.Load() > 0
| @@ -455,7 +455,7 @@ func (lru *LazyLRU[K, V]) MSetTTL(keys []K, values []V, ttl time.Duration) error | |||
| deathList = append(deathList, lru.setInternal(keys[i], values[i], expiration)...) | |||
| } | |||
| lru.lock.Unlock() | |||
| if len(deathList) > 0 && atomic.LoadInt32(&lru.numEvictCB) > 0 { | |||
| if len(deathList) > 0 && lru.numEvictCB.Load() > 0 { | |||
One or more callbacks can be registered to fire when items are removed from the cache. This is done synchronously to and Delete, Set, or MSet calls, as well as when the reaper thread finds expired items to remove. To keep the cost low, the count of registered callbacks is checked before any action is taken. For existing LazyLRU users, this will add the cost of one
atomic.LoadInt32to these operations. As compared to the locks that already exist, this is negligible.