From 34787f435732fc445d1d6a2bb67ce629c385f65e Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Thu, 17 Aug 2023 11:48:32 -0700 Subject: [PATCH 1/6] remove cold --- .../Lru/ConcurrentLruTests.cs | 14 +++---- BitFaster.Caching/Lru/ConcurrentLruCore.cs | 42 +++++++++++++++++++ 2 files changed, 47 insertions(+), 9 deletions(-) diff --git a/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs b/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs index dc22398c..e942a403 100644 --- a/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs +++ b/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs @@ -364,7 +364,7 @@ public class KeysInOrderTestDataGenerator : IEnumerable new object[] { new FavorWarmPartition(128, 0.6) }, new object[] { new FavorWarmPartition(256, 0.6) }, new object[] { new FavorWarmPartition(1024, 0.6) }, - //new object[] { new FavorWarmPartition(10*1024, 0.6) }, + new object[] { new FavorWarmPartition(10*1024, 0.6) }, //new object[] { new FavorWarmPartition(100*1024, 0.6) }, }; @@ -392,14 +392,10 @@ public void WhenKeysAreContinuouslyRequestedInTheOrderTheyAreAddedCountIsBounded for (int j = 0; j < i; j++) { lru.GetOrAdd(j, valueFactory.Create); - } - } - - // For larger cache sizes, I have observed capacity + 5. This is linked to the number of attempts. - // This is clearly a bug that needs further investigation, but considered not harmful at this point - // since growth is bounded, we just allow 4 more items than we should in the absolute worst case. - testOutputHelper.WriteLine($"Total: {lru.Count} Hot: {lru.HotCount} Warm: {lru.WarmCount} Cold: {lru.ColdCount}"); - lru.Count.Should().BeLessOrEqualTo(capacity + 1); + } + + lru.Count.Should().BeLessOrEqualTo(capacity + 1, $"Total: {lru.Count} Hot: {lru.HotCount} Warm: {lru.WarmCount} Cold: {lru.ColdCount}"); + } } [Fact] diff --git a/BitFaster.Caching/Lru/ConcurrentLruCore.cs b/BitFaster.Caching/Lru/ConcurrentLruCore.cs index fc0eac56..744e875a 100644 --- a/BitFaster.Caching/Lru/ConcurrentLruCore.cs +++ b/BitFaster.Caching/Lru/ConcurrentLruCore.cs @@ -557,6 +557,20 @@ private void Cycle(int hotCount) break; } } + + // 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 (/*attempts == maxAttempts &&*/ 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) + { + LastWarmToCold(); + } + + RemoveCold(ItemRemovedReason.Evicted); + } } else { @@ -566,6 +580,20 @@ private void Cycle(int hotCount) } } + private void LastWarmToCold() + { + Interlocked.Decrement(ref this.counter.warm); + + if (this.hotQueue.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 @@ -698,6 +726,20 @@ private void CycleDuringWarmup(int hotCount) } } + private void RemoveCold(ItemRemovedReason removedReason) + { + Interlocked.Decrement(ref this.counter.cold); + + if (this.coldQueue.TryDequeue(out var item)) + { + this.Move(item, ItemDestination.Remove, removedReason); + } + else + { + Interlocked.Increment(ref this.counter.cold); + } + } + [MethodImpl(MethodImplOptions.AggressiveInlining)] private int Move(I item, ItemDestination where, ItemRemovedReason removedReason) { From 6fd061b3f4e860b94f16f1055130349904645f03 Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Thu, 17 Aug 2023 12:33:34 -0700 Subject: [PATCH 2/6] cleanup --- BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs | 3 +-- BitFaster.Caching/Lru/ConcurrentLruCore.cs | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs b/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs index e942a403..ffce6b2e 100644 --- a/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs +++ b/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs @@ -364,8 +364,7 @@ public class KeysInOrderTestDataGenerator : IEnumerable new object[] { new FavorWarmPartition(128, 0.6) }, new object[] { new FavorWarmPartition(256, 0.6) }, new object[] { new FavorWarmPartition(1024, 0.6) }, - new object[] { new FavorWarmPartition(10*1024, 0.6) }, - //new object[] { new FavorWarmPartition(100*1024, 0.6) }, + new object[] { new FavorWarmPartition(16384, 0.6) }, }; public IEnumerator GetEnumerator() => _data.GetEnumerator(); diff --git a/BitFaster.Caching/Lru/ConcurrentLruCore.cs b/BitFaster.Caching/Lru/ConcurrentLruCore.cs index 744e875a..5e4f8fe8 100644 --- a/BitFaster.Caching/Lru/ConcurrentLruCore.cs +++ b/BitFaster.Caching/Lru/ConcurrentLruCore.cs @@ -561,7 +561,7 @@ private void Cycle(int hotCount) // 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 (/*attempts == maxAttempts &&*/ dest != ItemDestination.Remove) + 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) From 5cb72432e4b775336d12b39c85046f185f62505c Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Thu, 17 Aug 2023 14:31:21 -0700 Subject: [PATCH 3/6] disable long running test --- BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs b/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs index ffce6b2e..56ddcc62 100644 --- a/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs +++ b/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs @@ -364,7 +364,7 @@ public class KeysInOrderTestDataGenerator : IEnumerable new object[] { new FavorWarmPartition(128, 0.6) }, new object[] { new FavorWarmPartition(256, 0.6) }, new object[] { new FavorWarmPartition(1024, 0.6) }, - new object[] { new FavorWarmPartition(16384, 0.6) }, + // new object[] { new FavorWarmPartition(16384, 0.6) }, }; public IEnumerator GetEnumerator() => _data.GetEnumerator(); From 312a71f0f4ce7cc22a6d18a9cfdd83c0addf5245 Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Thu, 17 Aug 2023 14:49:36 -0700 Subject: [PATCH 4/6] rem --- BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs b/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs index 56ddcc62..76603e23 100644 --- a/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs +++ b/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs @@ -364,7 +364,6 @@ public class KeysInOrderTestDataGenerator : IEnumerable new object[] { new FavorWarmPartition(128, 0.6) }, new object[] { new FavorWarmPartition(256, 0.6) }, new object[] { new FavorWarmPartition(1024, 0.6) }, - // new object[] { new FavorWarmPartition(16384, 0.6) }, }; public IEnumerator GetEnumerator() => _data.GetEnumerator(); From b86cde369d7971a93f4469f5105d8228d2b566ad Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Thu, 17 Aug 2023 16:51:13 -0700 Subject: [PATCH 5/6] reverse cold/warm --- BitFaster.Caching/Lru/ConcurrentLruCore.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/BitFaster.Caching/Lru/ConcurrentLruCore.cs b/BitFaster.Caching/Lru/ConcurrentLruCore.cs index 5e4f8fe8..98e42e1e 100644 --- a/BitFaster.Caching/Lru/ConcurrentLruCore.cs +++ b/BitFaster.Caching/Lru/ConcurrentLruCore.cs @@ -542,13 +542,13 @@ private void Cycle(int hotCount) // 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)); + (dest, count) = CycleWarm(Volatile.Read(ref counter.cold)); if (dest != ItemDestination.Remove) { continue; } - (dest, count) = CycleWarm(Volatile.Read(ref counter.warm)); + (dest, count) = CycleCold(Volatile.Read(ref counter.warm)); if (dest != ItemDestination.Remove) { continue; From 8445b79a192f981330a9cf2084cda6246deaff1b Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Thu, 17 Aug 2023 18:05:29 -0700 Subject: [PATCH 6/6] reverse --- BitFaster.Caching/Lru/ConcurrentLruCore.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/BitFaster.Caching/Lru/ConcurrentLruCore.cs b/BitFaster.Caching/Lru/ConcurrentLruCore.cs index 98e42e1e..5e4f8fe8 100644 --- a/BitFaster.Caching/Lru/ConcurrentLruCore.cs +++ b/BitFaster.Caching/Lru/ConcurrentLruCore.cs @@ -542,13 +542,13 @@ private void Cycle(int hotCount) // so this process has races that could reduce cache size below capacity. This manifests // in 'off by one' which is considered harmless. - (dest, count) = CycleWarm(Volatile.Read(ref counter.cold)); + (dest, count) = CycleCold(Volatile.Read(ref counter.cold)); if (dest != ItemDestination.Remove) { continue; } - (dest, count) = CycleCold(Volatile.Read(ref counter.warm)); + (dest, count) = CycleWarm(Volatile.Read(ref counter.warm)); if (dest != ItemDestination.Remove) { continue;