From e93ca3f04c96e558111f51c2d0691239ee64d544 Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Thu, 9 Nov 2023 21:27:15 -0800 Subject: [PATCH 1/9] thru --- BitFaster.Caching/Lru/ConcurrentLruCore.cs | 43 +++++----------------- 1 file changed, 9 insertions(+), 34 deletions(-) diff --git a/BitFaster.Caching/Lru/ConcurrentLruCore.cs b/BitFaster.Caching/Lru/ConcurrentLruCore.cs index 7f89c50e..5bf83963 100644 --- a/BitFaster.Caching/Lru/ConcurrentLruCore.cs +++ b/BitFaster.Caching/Lru/ConcurrentLruCore.cs @@ -546,40 +546,13 @@ private void Cycle(int hotCount) { (var dest, var count) = CycleHot(hotCount); - const int maxAttempts = 5; - int attempts = 0; - - while (attempts++ < maxAttempts) + if (dest == ItemDestination.Warm) { - if (dest == ItemDestination.Warm) - { - (dest, count) = CycleWarm(count); - } - else if (dest == ItemDestination.Cold) - { - (dest, count) = CycleCold(count); - } - else - { - // If an item was removed, it is possible that the warm and cold queues are still oversize. - // Attempt to recover. It is possible that multiple threads read the same queue count here, - // so this process has races that could reduce cache size below capacity. This manifests - // in 'off by one' which is considered harmless. - - (dest, count) = CycleCold(Volatile.Read(ref counter.cold)); - if (dest != ItemDestination.Remove) - { - continue; - } - - (dest, count) = CycleWarm(Volatile.Read(ref counter.warm)); - if (dest != ItemDestination.Remove) - { - continue; - } - - break; - } + (dest, count) = CycleWarm(count); + } + else if (dest == ItemDestination.Cold) + { + (dest, count) = CycleCold(count); } // If we get here, we have cycled the queues multiple times and still have not removed an item. @@ -604,6 +577,7 @@ private void Cycle(int hotCount) } } + [MethodImpl(MethodImplOptions.AggressiveInlining)] private void LastWarmToCold() { Interlocked.Decrement(ref this.counter.warm); @@ -750,6 +724,7 @@ private void CycleDuringWarmup(int hotCount) } } + [MethodImpl(MethodImplOptions.AggressiveInlining)] private void RemoveCold(ItemRemovedReason removedReason) { Interlocked.Decrement(ref this.counter.cold); @@ -789,7 +764,7 @@ private int Move(I item, ItemDestination where, ItemRemovedReason removedReason) this.telemetryPolicy.OnItemRemoved(item.Key, item.Value, removedReason); - lock (item) + //lock (item) { Disposer.Dispose(item.Value); } From 02d2316322634b2fd8f88509c6bb0e13377b73c3 Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Thu, 9 Nov 2023 21:31:12 -0800 Subject: [PATCH 2/9] lock --- BitFaster.Caching/Lru/ConcurrentLruCore.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/BitFaster.Caching/Lru/ConcurrentLruCore.cs b/BitFaster.Caching/Lru/ConcurrentLruCore.cs index 5bf83963..cadb0da4 100644 --- a/BitFaster.Caching/Lru/ConcurrentLruCore.cs +++ b/BitFaster.Caching/Lru/ConcurrentLruCore.cs @@ -764,7 +764,7 @@ private int Move(I item, ItemDestination where, ItemRemovedReason removedReason) this.telemetryPolicy.OnItemRemoved(item.Key, item.Value, removedReason); - //lock (item) + lock (item) { Disposer.Dispose(item.Value); } From 5c99171b7c60685a0e7e18d3ba427684010c42bd Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Thu, 9 Nov 2023 21:42:01 -0800 Subject: [PATCH 3/9] debug view --- BitFaster.Caching/Lru/ConcurrentLruCore.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/BitFaster.Caching/Lru/ConcurrentLruCore.cs b/BitFaster.Caching/Lru/ConcurrentLruCore.cs index cadb0da4..a8ab48ca 100644 --- a/BitFaster.Caching/Lru/ConcurrentLruCore.cs +++ b/BitFaster.Caching/Lru/ConcurrentLruCore.cs @@ -801,7 +801,12 @@ private static CachePolicy CreatePolicy(ConcurrentLruCore lru) // it becomes immutable. However, this object is then somewhere else on the // heap, which slows down the policies with hit counter logic in benchmarks. Likely // this approach keeps the structs data members in the same CPU cache line as the LRU. + // backcompat: remove conditional compile +#if NETCOREAPP3_0_OR_GREATER [DebuggerDisplay("Hit = {Hits}, Miss = {Misses}, Upd = {Updated}, Evict = {Evicted}")] +#else + [DebuggerDisplay("Hit = {Hits}, Miss = {Misses}, Evict = {Evicted}")] +#endif private class Proxy : ICacheMetrics, ICacheEvents, IBoundedPolicy, ITimePolicy { private readonly ConcurrentLruCore lru; From c4f1f9dbbab634761668aa290b22825e6bb2b96e Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Thu, 9 Nov 2023 21:52:54 -0800 Subject: [PATCH 4/9] bounded --- BitFaster.Caching/Lru/ConcurrentLruCore.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/BitFaster.Caching/Lru/ConcurrentLruCore.cs b/BitFaster.Caching/Lru/ConcurrentLruCore.cs index a8ab48ca..e9657a14 100644 --- a/BitFaster.Caching/Lru/ConcurrentLruCore.cs +++ b/BitFaster.Caching/Lru/ConcurrentLruCore.cs @@ -566,7 +566,7 @@ private void Cycle(int hotCount) LastWarmToCold(); } - RemoveCold(ItemRemovedReason.Evicted); + ConstrainCold(ItemRemovedReason.Evicted); } } else @@ -725,13 +725,13 @@ private void CycleDuringWarmup(int hotCount) } [MethodImpl(MethodImplOptions.AggressiveInlining)] - private void RemoveCold(ItemRemovedReason removedReason) + private void ConstrainCold(ItemRemovedReason removedReason) { - Interlocked.Decrement(ref this.counter.cold); + int cc = Interlocked.Decrement(ref this.counter.cold); - if (this.coldQueue.TryDequeue(out var item)) + if (cc == this.capacity.Cold && this.coldQueue.TryDequeue(out var item)) { - this.Move(item, ItemDestination.Remove, removedReason); + this.Move(item, ItemDestination.Remove, removedReason); } else { From 408ca66872a972a8183641831792ac11c71080e7 Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Thu, 9 Nov 2023 22:14:16 -0800 Subject: [PATCH 5/9] use counter --- BitFaster.Caching/Lru/ConcurrentLruCore.cs | 41 ++++++++++------------ 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/BitFaster.Caching/Lru/ConcurrentLruCore.cs b/BitFaster.Caching/Lru/ConcurrentLruCore.cs index e9657a14..410e6fa8 100644 --- a/BitFaster.Caching/Lru/ConcurrentLruCore.cs +++ b/BitFaster.Caching/Lru/ConcurrentLruCore.cs @@ -561,12 +561,12 @@ private void Cycle(int hotCount) if (dest != ItemDestination.Remove) { // if an item was last moved into warm, move the last warm item to cold to prevent enlarging warm - if (dest == ItemDestination.Warm) + if (dest == ItemDestination.Warm && count > this.capacity.Warm) { - LastWarmToCold(); + count = LastWarmToCold(); } - ConstrainCold(ItemRemovedReason.Evicted); + ConstrainCold(count, ItemRemovedReason.Evicted); } } else @@ -577,21 +577,6 @@ private void Cycle(int hotCount) } } - [MethodImpl(MethodImplOptions.AggressiveInlining)] - private void LastWarmToCold() - { - Interlocked.Decrement(ref this.counter.warm); - - if (this.warmQueue.TryDequeue(out var item)) - { - this.Move(item, ItemDestination.Cold, ItemRemovedReason.Evicted); - } - else - { - Interlocked.Increment(ref this.counter.warm); - } - } - private void CycleDuringWarmup(int hotCount) { // do nothing until hot is full @@ -725,17 +710,27 @@ private void CycleDuringWarmup(int hotCount) } [MethodImpl(MethodImplOptions.AggressiveInlining)] - private void ConstrainCold(ItemRemovedReason removedReason) + private int LastWarmToCold() { - int cc = Interlocked.Decrement(ref this.counter.cold); + Interlocked.Decrement(ref this.counter.warm); - if (cc == this.capacity.Cold && this.coldQueue.TryDequeue(out var item)) + if (this.warmQueue.TryDequeue(out var item)) { - this.Move(item, ItemDestination.Remove, removedReason); + return this.Move(item, ItemDestination.Cold, ItemRemovedReason.Evicted); } else { - Interlocked.Increment(ref this.counter.cold); + Interlocked.Increment(ref this.counter.warm); + return 0; + } + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private void ConstrainCold(int coldCount, ItemRemovedReason removedReason) + { + if (coldCount > this.capacity.Cold && this.coldQueue.TryDequeue(out var item)) + { + this.Move(item, ItemDestination.Remove, removedReason); } } From 5d2bd9e27a8bdd7ac28ef45837805579411d43e9 Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Thu, 9 Nov 2023 22:27:10 -0800 Subject: [PATCH 6/9] fix cold counter --- BitFaster.Caching/Lru/ConcurrentLruCore.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/BitFaster.Caching/Lru/ConcurrentLruCore.cs b/BitFaster.Caching/Lru/ConcurrentLruCore.cs index 410e6fa8..31e460d8 100644 --- a/BitFaster.Caching/Lru/ConcurrentLruCore.cs +++ b/BitFaster.Caching/Lru/ConcurrentLruCore.cs @@ -555,12 +555,9 @@ private void Cycle(int hotCount) (dest, count) = CycleCold(count); } - // If we get here, we have cycled the queues multiple times and still have not removed an item. - // This can happen if the cache is full of items that are not discardable. In this case, we simply - // discard the coldest item to avoid unbounded growth. + // If nothing was removed yet, constrain the size of warm and cold by discarding the coldest item. if (dest != ItemDestination.Remove) { - // if an item was last moved into warm, move the last warm item to cold to prevent enlarging warm if (dest == ItemDestination.Warm && count > this.capacity.Warm) { count = LastWarmToCold(); @@ -730,6 +727,7 @@ private void ConstrainCold(int coldCount, ItemRemovedReason removedReason) { if (coldCount > this.capacity.Cold && this.coldQueue.TryDequeue(out var item)) { + Interlocked.Decrement(ref this.counter.cold); this.Move(item, ItemDestination.Remove, removedReason); } } From 64f0c3c9ac39153415d36572c9e57689c1a46261 Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Fri, 10 Nov 2023 09:01:04 -0800 Subject: [PATCH 7/9] consolidate remove logic --- BitFaster.Caching/Lru/ConcurrentLruCore.cs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/BitFaster.Caching/Lru/ConcurrentLruCore.cs b/BitFaster.Caching/Lru/ConcurrentLruCore.cs index 31e460d8..1f3148fe 100644 --- a/BitFaster.Caching/Lru/ConcurrentLruCore.cs +++ b/BitFaster.Caching/Lru/ConcurrentLruCore.cs @@ -348,6 +348,7 @@ public bool TryRemove(K key) return TryRemove(key, out _); } + [MethodImpl(MethodImplOptions.AggressiveInlining)] private void OnRemove(K key, I item) { // Mark as not accessed, it will later be cycled out of the queues because it can never be fetched @@ -365,7 +366,6 @@ private void OnRemove(K key, I item) } } - /// ///Note: Calling this method does not affect LRU order. public bool TryUpdate(K key, V value) @@ -749,18 +749,14 @@ private int Move(I item, ItemDestination where, ItemRemovedReason removedReason) var kvp = new KeyValuePair(item.Key, item); - // hidden atomic remove +#if NET6_0_OR_GREATER + if (this.dictionary.TryRemove(kvp)) +#else // https://devblogs.microsoft.com/pfxteam/little-known-gems-atomic-conditional-removals-from-concurrentdictionary/ if (((ICollection>)this.dictionary).Remove(kvp)) +#endif { - item.WasRemoved = true; - - this.telemetryPolicy.OnItemRemoved(item.Key, item.Value, removedReason); - - lock (item) - { - Disposer.Dispose(item.Value); - } + OnRemove(item.Key, item); } break; } From 0f06c91dceea5d8a0c3a1cf999707641d65a4c2a Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Fri, 10 Nov 2023 09:14:47 -0800 Subject: [PATCH 8/9] reason --- BitFaster.Caching/Lru/ConcurrentLruCore.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/BitFaster.Caching/Lru/ConcurrentLruCore.cs b/BitFaster.Caching/Lru/ConcurrentLruCore.cs index 1f3148fe..d8f6d131 100644 --- a/BitFaster.Caching/Lru/ConcurrentLruCore.cs +++ b/BitFaster.Caching/Lru/ConcurrentLruCore.cs @@ -312,7 +312,7 @@ public bool TryRemove(KeyValuePair item) if (((ICollection>)this.dictionary).Remove(kvp)) #endif { - OnRemove(item.Key, kvp.Value); + OnRemove(item.Key, kvp.Value, ItemRemovedReason.Removed); return true; } } @@ -333,7 +333,7 @@ public bool TryRemove(K key, out V value) { if (this.dictionary.TryRemove(key, out var item)) { - OnRemove(key, item); + OnRemove(key, item, ItemRemovedReason.Removed); value = item.Value; return true; } @@ -349,7 +349,7 @@ public bool TryRemove(K key) } [MethodImpl(MethodImplOptions.AggressiveInlining)] - private void OnRemove(K key, I item) + private void OnRemove(K key, I item, ItemRemovedReason reason) { // Mark as not accessed, it will later be cycled out of the queues because it can never be fetched // from the dictionary. Note: Hot/Warm/Cold count will reflect the removed item until it is cycled @@ -357,7 +357,7 @@ private void OnRemove(K key, I item) item.WasAccessed = false; item.WasRemoved = true; - this.telemetryPolicy.OnItemRemoved(key, item.Value, ItemRemovedReason.Removed); + this.telemetryPolicy.OnItemRemoved(key, item.Value, reason); // serialize dispose (common case dispose not thread safe) lock (item) @@ -756,7 +756,7 @@ private int Move(I item, ItemDestination where, ItemRemovedReason removedReason) if (((ICollection>)this.dictionary).Remove(kvp)) #endif { - OnRemove(item.Key, item); + OnRemove(item.Key, item, removedReason); } break; } From 006bb46920024f82eb98069bf28b003c6d2d194d Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Fri, 10 Nov 2023 14:25:31 -0800 Subject: [PATCH 9/9] more cycles --- BitFaster.Caching/Lru/ConcurrentLruCore.cs | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/BitFaster.Caching/Lru/ConcurrentLruCore.cs b/BitFaster.Caching/Lru/ConcurrentLruCore.cs index d8f6d131..64ed80fa 100644 --- a/BitFaster.Caching/Lru/ConcurrentLruCore.cs +++ b/BitFaster.Caching/Lru/ConcurrentLruCore.cs @@ -546,15 +546,19 @@ private void Cycle(int hotCount) { (var dest, var count) = CycleHot(hotCount); - if (dest == ItemDestination.Warm) + int cycles = 0; + while (cycles++ < 3 && dest != ItemDestination.Remove) { - (dest, count) = CycleWarm(count); - } - else if (dest == ItemDestination.Cold) - { - (dest, count) = CycleCold(count); + if (dest == ItemDestination.Warm) + { + (dest, count) = CycleWarm(count); + } + else if (dest == ItemDestination.Cold) + { + (dest, count) = CycleCold(count); + } } - + // If nothing was removed yet, constrain the size of warm and cold by discarding the coldest item. if (dest != ItemDestination.Remove) {