From 175e9d598ec48079ace8b54bba76b462c3fbe779 Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Mon, 6 Nov 2023 19:10:04 -0800 Subject: [PATCH] Fix LRU clear when warm/hot items are touched (#453) * Fix LRU clear when warm/hot items are touched * add trim test * special case * cleanup --------- --- .../Lru/ConcurrentLruTests.cs | 101 +++++++++++++++++- BitFaster.Caching/Lru/ConcurrentLruCore.cs | 34 +++--- 2 files changed, 114 insertions(+), 21 deletions(-) diff --git a/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs b/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs index 5395700e..4c5d5266 100644 --- a/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs +++ b/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs @@ -9,7 +9,6 @@ using Xunit.Abstractions; using System.Collections.Concurrent; using System.Reflection; -using System.Runtime.CompilerServices; namespace BitFaster.Caching.UnitTests.Lru { @@ -875,6 +874,67 @@ public void WhenItemsExistClearRemovesAllItems() lru.ColdCount.Should().Be(0); } + // This is a special case: + // Cycle 1: hot => warm + // Cycle 2: warm => warm + // Cycle 3: warm => cold + // Cycle 4: cold => remove + // Cycle 5: cold => remove + [Fact] + public void WhenCacheIsSize3ItemsExistAndItemsAccessedClearRemovesAllItems() + { + lru = new ConcurrentLru(3); + + lru.AddOrUpdate(1, "1"); + lru.AddOrUpdate(2, "1"); + + lru.TryGet(1, out _); + lru.TryGet(2, out _); + + lru.Clear(); + + lru.Count.Should().Be(0); + } + + [Theory] + [InlineData(1)] + [InlineData(2)] + [InlineData(3)] + [InlineData(4)] + [InlineData(5)] + [InlineData(6)] + [InlineData(7)] + [InlineData(8)] + [InlineData(9)] + [InlineData(10)] + public void WhenItemsExistAndItemsAccessedClearRemovesAllItems(int itemCount) + { + // By default capacity is 9. Test all possible states of touched items + // in the cache. + + for (int i = 0; i < itemCount; i++) + { + lru.AddOrUpdate(i, "1"); + } + + // touch n items + for (int i = 0; i < itemCount; i++) + { + lru.TryGet(i, out _); + } + + lru.Clear(); + + this.testOutputHelper.WriteLine("LRU " + string.Join(" ", lru.Keys)); + + lru.Count.Should().Be(0); + + // verify queues are purged + lru.HotCount.Should().Be(0); + lru.WarmCount.Should().Be(0); + lru.ColdCount.Should().Be(0); + } + [Fact] public void WhenWarmThenClearedIsWarmIsReset() { @@ -1079,6 +1139,45 @@ public void WhenColdItemsAreTouchedTrimRemovesExpectedItemCount(int trimCount, i this.testOutputHelper.WriteLine("exp " + string.Join(" ", expected)); lru.Keys.Should().BeEquivalentTo(expected); + } + + [Theory] + [InlineData(1)] + [InlineData(2)] + [InlineData(3)] + [InlineData(4)] + [InlineData(5)] + [InlineData(6)] + [InlineData(7)] + [InlineData(8)] + [InlineData(9)] + [InlineData(10)] + public void WhenItemsExistAndItemsAccessedTrimRemovesAllItems(int itemCount) + { + // By default capacity is 9. Test all possible states of touched items + // in the cache. + + for (int i = 0; i < itemCount; i++) + { + lru.AddOrUpdate(i, "1"); + } + + // touch n items + for (int i = 0; i < itemCount; i++) + { + lru.TryGet(i, out _); + } + + lru.Trim(Math.Min(itemCount, lru.Capacity)); + + this.testOutputHelper.WriteLine("LRU " + string.Join(" ", lru.Keys)); + + lru.Count.Should().Be(0); + + // verify queues are purged + lru.HotCount.Should().Be(0); + lru.WarmCount.Should().Be(0); + lru.ColdCount.Should().Be(0); } [Fact] diff --git a/BitFaster.Caching/Lru/ConcurrentLruCore.cs b/BitFaster.Caching/Lru/ConcurrentLruCore.cs index c4024929..7f89c50e 100644 --- a/BitFaster.Caching/Lru/ConcurrentLruCore.cs +++ b/BitFaster.Caching/Lru/ConcurrentLruCore.cs @@ -422,15 +422,7 @@ public void AddOrUpdate(K key, V value) /// public void Clear() { - int count = this.Count; - - for (int i = 0; i < count; i++) - { - CycleHotUnchecked(ItemRemovedReason.Cleared); - CycleWarmUnchecked(ItemRemovedReason.Cleared); - TryRemoveCold(ItemRemovedReason.Cleared); - } - + this.TrimLiveItems(itemsRemoved: 0, this.Count, ItemRemovedReason.Cleared); Volatile.Write(ref this.isWarm, false); } @@ -458,7 +450,7 @@ public void Trim(int itemCount) // first scan each queue for discardable items and remove them immediately. Note this can remove > itemCount items. int itemsRemoved = this.itemPolicy.CanDiscard() ? TrimAllDiscardedItems() : 0; - TrimLiveItems(itemsRemoved, itemCount); + TrimLiveItems(itemsRemoved, itemCount, ItemRemovedReason.Trimmed); } private void TrimExpired() @@ -507,42 +499,44 @@ void RemoveDiscardableItems(ConcurrentQueue q, ref int queueCounter) return itemsRemoved; } - private void TrimLiveItems(int itemsRemoved, int itemCount) + private void TrimLiveItems(int itemsRemoved, int itemCount, ItemRemovedReason reason) { + // When items are touched, they are moved to warm by cycling. Therefore, to guarantee + // that we can remove itemCount items, we must cycle (2 * capacity.Warm) + capacity.Hot times. // If clear is called during trimming, it would be possible to get stuck in an infinite - // loop here. Instead quit after n consecutive failed attempts to move warm/hot to cold. + // loop here. The warm + hot limit also guards against this case. int trimWarmAttempts = 0; - int maxAttempts = this.capacity.Cold + 1; + int maxWarmHotAttempts = (this.capacity.Warm * 2) + this.capacity.Hot; - while (itemsRemoved < itemCount && trimWarmAttempts < maxAttempts) + while (itemsRemoved < itemCount && trimWarmAttempts < maxWarmHotAttempts) { if (Volatile.Read(ref this.counter.cold) > 0) { - if (TryRemoveCold(ItemRemovedReason.Trimmed) == (ItemDestination.Remove, 0)) + if (TryRemoveCold(reason) == (ItemDestination.Remove, 0)) { itemsRemoved++; trimWarmAttempts = 0; } - TrimWarmOrHot(); + TrimWarmOrHot(reason); } else { - TrimWarmOrHot(); + TrimWarmOrHot(reason); trimWarmAttempts++; } } } - private void TrimWarmOrHot() + private void TrimWarmOrHot(ItemRemovedReason reason) { if (Volatile.Read(ref this.counter.warm) > 0) { - CycleWarmUnchecked(ItemRemovedReason.Trimmed); + CycleWarmUnchecked(reason); } else if (Volatile.Read(ref this.counter.hot) > 0) { - CycleHotUnchecked(ItemRemovedReason.Trimmed); + CycleHotUnchecked(reason); } }