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 f44e27fbe63d189807c18c9f63c505bb3d4d60e2 Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Thu, 9 Nov 2023 23:25:34 -0800 Subject: [PATCH 7/9] stable warm --- BitFaster.Caching/Lru/ConcurrentLruCore.cs | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/BitFaster.Caching/Lru/ConcurrentLruCore.cs b/BitFaster.Caching/Lru/ConcurrentLruCore.cs index 31e460d8..c57f2966 100644 --- a/BitFaster.Caching/Lru/ConcurrentLruCore.cs +++ b/BitFaster.Caching/Lru/ConcurrentLruCore.cs @@ -583,20 +583,14 @@ private void CycleDuringWarmup(int hotCount) if (this.hotQueue.TryDequeue(out var item)) { - // always move to warm until it is full - if (Volatile.Read(ref this.counter.warm) < this.capacity.Warm) - { - // If there is a race, we will potentially add multiple items to warm. Guard by cycling the queue. - int warmCount = this.Move(item, ItemDestination.Warm, ItemRemovedReason.Evicted); - CycleWarm(warmCount); - } - else + int count = this.Move(item, ItemDestination.Warm, ItemRemovedReason.Evicted); + + // if warm is now full, overflow to cold and mark as warm + if (count > this.capacity.Warm) { - // Else mark isWarm and move items to cold. - // If there is a race, we will potentially add multiple items to cold. Guard by cycling the queue. Volatile.Write(ref this.isWarm, true); - int coldCount = this.Move(item, ItemDestination.Cold, ItemRemovedReason.Evicted); - CycleCold(coldCount); + count = LastWarmToCold(); + ConstrainCold(count, ItemRemovedReason.Evicted); } } else From ccad565e43b79699feb287cc6542aa2c02853bdc Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Fri, 10 Nov 2023 15:21:33 -0800 Subject: [PATCH 8/9] align tests --- .../Lru/ConcurrentLruTests.cs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs b/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs index 4c5d5266..b891225f 100644 --- a/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs +++ b/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs @@ -562,11 +562,10 @@ public void WhenValueExpiresItIsDisposed() lruOfDisposable.GetOrAdd(i, disposableValueFactory.Create); } - disposableValueFactory.Items[0].IsDisposed.Should().BeFalse(); - disposableValueFactory.Items[1].IsDisposed.Should().BeFalse(); - - disposableValueFactory.Items[2].IsDisposed.Should().BeTrue(); + disposableValueFactory.Items[0].IsDisposed.Should().BeTrue(); + disposableValueFactory.Items[1].IsDisposed.Should().BeFalse(); + disposableValueFactory.Items[2].IsDisposed.Should().BeFalse(); disposableValueFactory.Items[3].IsDisposed.Should().BeFalse(); disposableValueFactory.Items[4].IsDisposed.Should().BeFalse(); disposableValueFactory.Items[5].IsDisposed.Should().BeFalse(); @@ -598,8 +597,8 @@ public void WhenValueEvictedItemRemovedEventIsFired() removedItems.Count.Should().Be(2); - removedItems[0].Key.Should().Be(3); - removedItems[0].Value.Should().Be(4); + removedItems[0].Key.Should().Be(1); + removedItems[0].Value.Should().Be(2); removedItems[0].Reason.Should().Be(ItemRemovedReason.Evicted); removedItems[1].Key.Should().Be(4); @@ -1016,6 +1015,8 @@ public void WhenTrimCountIsMoreThanCapacityThrows() [InlineData(9, new int[] { })] public void WhenColdItemsExistTrimRemovesExpectedItemCount(int trimCount, int[] expected) { + Warmup(); + // initial state: // Hot = 9, 8, 7 // Warm = 3, 2, 1 @@ -1109,6 +1110,8 @@ public void WhenHotItemsExistTrimRemovesExpectedItemCount(int itemCount, int[] e [InlineData(9, new int[] { })] public void WhenColdItemsAreTouchedTrimRemovesExpectedItemCount(int trimCount, int[] expected) { + Warmup(); + // initial state: // Hot = 9, 8, 7 // Warm = 3, 2, 1 From fd530623894d78e9894a19595ebda8c2592be6fb Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Fri, 10 Nov 2023 15:28:37 -0800 Subject: [PATCH 9/9] tlru tests --- BitFaster.Caching.UnitTests/Lru/ConcurrentTLruTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/BitFaster.Caching.UnitTests/Lru/ConcurrentTLruTests.cs b/BitFaster.Caching.UnitTests/Lru/ConcurrentTLruTests.cs index 16822234..d12eab39 100644 --- a/BitFaster.Caching.UnitTests/Lru/ConcurrentTLruTests.cs +++ b/BitFaster.Caching.UnitTests/Lru/ConcurrentTLruTests.cs @@ -92,8 +92,8 @@ public void WhenValueEvictedItemRemovedEventIsFired() removedItems.Count.Should().Be(2); - removedItems[0].Key.Should().Be(3); - removedItems[0].Value.Should().Be(4); + removedItems[0].Key.Should().Be(1); + removedItems[0].Value.Should().Be(2); removedItems[0].Reason.Should().Be(ItemRemovedReason.Evicted); removedItems[1].Key.Should().Be(4);