From fb2ee06c65f20183d43e5ab1d7bd0f8c65d43224 Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Mon, 25 Jul 2022 16:37:51 -0600 Subject: [PATCH 1/7] Improve comments --- .../src/MemoryCache.cs | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index 0bb304273877a0..6844186533e040 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -76,7 +76,12 @@ public MemoryCache(IOptions optionsAccessor, ILoggerFactory /// public int Count => _coherentState.Count; - // internal for testing + /// + /// Internal accessor for Size for testing only. + /// + /// Note that this is only eventually consistent with the contents of the collection. + /// See comment on . + /// internal long Size => _coherentState.Size; internal bool TrackLinkedCacheEntries { get; } @@ -613,6 +618,22 @@ private static void ValidateCacheKey(object key) ThrowHelper.ThrowIfNull(key); } + /// + /// Wrapper for the memory cache entries collection. + /// + /// Entries may have various sizes. If a size limit has been set, the cache keeps track of the aggregate of all the entries' sizes + /// in order to trigger compaction when the size limit is exceeded. + /// Because the overall size is used only to trigger compaction, it is not necessary to update it atomically with the collection, + /// so long as it is always eventually consistent. So instead of locking around updates to the collection and the overall size, + /// the size is updated after the collection, using an Interlocked operation. + /// + /// When the memory cache is cleared, it replaces the backing collection entirely. This may occur in parallel with operations + /// like add, set, remove, and compact which may modify the collection and thus its overall size. + /// To keep the overall size eventually consistent, therefore, the collection and the overall size are wrapped in this CoherentState + /// object. Individual operations take a local reference to this wrapper object while they work, and make size updates to this object. + /// Clearing the cache simply replaces the object, so that any still in progress updates do not affect the overall size value for + /// the new backing collection. + /// private sealed class CoherentState { internal ConcurrentDictionary _entries = new ConcurrentDictionary(); From 25cb29bce3fef71c7076df9b628ea35bee28f68a Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Mon, 25 Jul 2022 16:57:14 -0600 Subject: [PATCH 2/7] Re-enable tests disabled for 33993 --- .../tests/CapacityTests.cs | 5 ----- .../tests/MemoryCacheSetAndRemoveTests.cs | 3 --- 2 files changed, 8 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CapacityTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CapacityTests.cs index 79e09aaf54449c..a5e5822a96b92c 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CapacityTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CapacityTests.cs @@ -110,7 +110,6 @@ public void DoNotAddEntryIfItExceedsCapacity() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/33993")] public async Task DoNotAddIfSizeOverflows() { var cache = new MemoryCache(new MemoryCacheOptions { SizeLimit = long.MaxValue }); @@ -143,7 +142,6 @@ public async Task DoNotAddIfSizeOverflows() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/33993")] public async Task ExceedsCapacityCompacts() { var cache = new MemoryCache(new MemoryCacheOptions @@ -237,7 +235,6 @@ public void AddingReplacementWhenTotalSizeExceedsCapacityDoesNotUpdateAndRemoves } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/33993")] public async Task AddingReplacementWhenTotalSizeExceedsCapacityDoesNotUpdateRemovesOldEntryAndTriggersCompaction() { var cache = new MemoryCache(new MemoryCacheOptions @@ -307,7 +304,6 @@ public void RemovingEntryDecreasesCacheSize() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/33993")] public async Task ExpiringEntryDecreasesCacheSize() { var cache = new MemoryCache(new MemoryCacheOptions @@ -378,7 +374,6 @@ public void TryingToAddEntryWithExpiredTokenDoesNotIncreaseCacheSize() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/33993")] public async Task CompactsToLessThanLowWatermarkUsingLRUWhenHighWatermarkExceeded() { var testClock = new TestClock(); diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs index 4ac0e73ceca71b..5e79dac863dc0e 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs @@ -543,7 +543,6 @@ public void SetGetAndRemoveWorksWithObjectKeysWhenDifferentReferences() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/33993")] public void GetAndSet_AreThreadSafe_AndUpdatesNeverLeavesNullValues() { var cache = CreateCache(); @@ -597,7 +596,6 @@ public void GetAndSet_AreThreadSafe_AndUpdatesNeverLeavesNullValues() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/33993")] public void OvercapacityPurge_AreThreadSafe() { var cache = new MemoryCache(new MemoryCacheOptions @@ -663,7 +661,6 @@ public void OvercapacityPurge_AreThreadSafe() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/33993")] public void AddAndReplaceEntries_AreThreadSafe() { var cache = new MemoryCache(new MemoryCacheOptions From 739628052ce6902a966528ddd161b4fc7f9af85e Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Mon, 25 Jul 2022 17:47:51 -0600 Subject: [PATCH 3/7] Add comment --- .../Microsoft.Extensions.Caching.Abstractions/src/ICacheEntry.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/Microsoft.Extensions.Caching.Abstractions/src/ICacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Abstractions/src/ICacheEntry.cs index 02ff52ee5942a2..7ea2c8c8595b3b 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Abstractions/src/ICacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Abstractions/src/ICacheEntry.cs @@ -9,6 +9,7 @@ namespace Microsoft.Extensions.Caching.Memory { /// /// Represents an entry in the implementation. + /// When Disposed, is committed to the cache. /// public interface ICacheEntry : IDisposable { From dbf6069bda157d399c6b2efba5ca0b8cf17b107a Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Mon, 25 Jul 2022 17:55:51 -0600 Subject: [PATCH 4/7] Fix tests using retries --- .../tests/CapacityTests.cs | 88 +++++++++++-------- .../tests/MemoryCacheSetAndRemoveTests.cs | 4 +- 2 files changed, 51 insertions(+), 41 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CapacityTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CapacityTests.cs index a5e5822a96b92c..31a92dddeb0e32 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CapacityTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CapacityTests.cs @@ -72,11 +72,11 @@ public void AddingEntryIncreasesCacheSizeWhenEnforcingSizeLimit() { var cache = new MemoryCache(new MemoryCacheOptions { SizeLimit = 10 }); - Assert.Equal(0, cache.Size); + AssertCacheSize(0, cache); cache.Set("key", "value", new MemoryCacheEntryOptions { Size = 5 }); - Assert.Equal(5, cache.Size); + AssertCacheSize(5, cache); } [Fact] @@ -84,11 +84,11 @@ public void AddingEntryDoesNotIncreasesCacheSizeWhenNotEnforcingSizeLimit() { var cache = new MemoryCache(new MemoryCacheOptions()); - Assert.Equal(0, cache.Size); + AssertCacheSize(0, cache); cache.Set("key", "value", new MemoryCacheEntryOptions { Size = 5 }); - Assert.Equal(0, cache.Size); + AssertCacheSize(0, cache); } [Fact] @@ -96,17 +96,17 @@ public void DoNotAddEntryIfItExceedsCapacity() { var cache = new MemoryCache(new MemoryCacheOptions { SizeLimit = 10 }); - Assert.Equal(0, cache.Size); + AssertCacheSize(0, cache); cache.Set("key", "value", new MemoryCacheEntryOptions { Size = 4 }); Assert.Equal("value", cache.Get("key")); - Assert.Equal(4, cache.Size); + AssertCacheSize(4, cache); cache.Set("key2", "value2", new MemoryCacheEntryOptions { Size = 7 }); Assert.Null(cache.Get("key2")); - Assert.Equal(4, cache.Size); + AssertCacheSize(4, cache); } [Fact] @@ -122,12 +122,12 @@ public async Task DoNotAddIfSizeOverflows() State = null }); - Assert.Equal(0, cache.Size); + AssertCacheSize(0, cache); cache.Set("key", "value", entryOptions); Assert.Equal("value", cache.Get("key")); - Assert.Equal(long.MaxValue, cache.Size); + AssertCacheSize(long.MaxValue, cache); cache.Set("key1", "value1", new MemoryCacheEntryOptions { Size = long.MaxValue }); // Do not add the new item @@ -138,7 +138,7 @@ public async Task DoNotAddIfSizeOverflows() // Compaction removes old item Assert.Null(cache.Get("key")); - Assert.Equal(0, cache.Size); + AssertCacheSize(0, cache); } [Fact] @@ -159,12 +159,12 @@ public async Task ExceedsCapacityCompacts() State = null }); - Assert.Equal(0, cache.Size); + AssertCacheSize(0, cache); cache.Set("key", "value", entryOptions); Assert.Equal("value", cache.Get("key")); - Assert.Equal(6, cache.Size); + AssertCacheSize(6, cache); cache.Set("key2", "value2", new MemoryCacheEntryOptions { Size = 5 }); @@ -173,7 +173,7 @@ public async Task ExceedsCapacityCompacts() Assert.Null(cache.Get("key")); Assert.Null(cache.Get("key2")); - Assert.Equal(0, cache.Size); + AssertCacheSize(0, cache); } [Fact] @@ -181,17 +181,17 @@ public void AddingReplacementWithSizeIncreaseUpdates() { var cache = new MemoryCache(new MemoryCacheOptions { SizeLimit = 10 }); - Assert.Equal(0, cache.Size); + AssertCacheSize(0, cache); cache.Set("key", "value", new MemoryCacheEntryOptions { Size = 2 }); Assert.Equal("value", cache.Get("key")); - Assert.Equal(2, cache.Size); + AssertCacheSize(2, cache); cache.Set("key", "value1", new MemoryCacheEntryOptions { Size = 3 }); Assert.Equal("value1", cache.Get("key")); - Assert.Equal(3, cache.Size); + AssertCacheSize(3, cache); } [Fact] @@ -199,17 +199,17 @@ public void AddingReplacementWithSizeDecreaseUpdates() { var cache = new MemoryCache(new MemoryCacheOptions { SizeLimit = 10 }); - Assert.Equal(0, cache.Size); + AssertCacheSize(0, cache); cache.Set("key", "value", new MemoryCacheEntryOptions { Size = 2 }); Assert.Equal("value", cache.Get("key")); - Assert.Equal(2, cache.Size); + AssertCacheSize(2, cache); cache.Set("key", "value1", new MemoryCacheEntryOptions { Size = 1 }); Assert.Equal("value1", cache.Get("key")); - Assert.Equal(1, cache.Size); + AssertCacheSize(1, cache); } [Fact] @@ -221,17 +221,17 @@ public void AddingReplacementWhenTotalSizeExceedsCapacityDoesNotUpdateAndRemoves CompactionPercentage = 0.5 }); - Assert.Equal(0, cache.Size); + AssertCacheSize(0, cache); cache.Set("key", "value", new MemoryCacheEntryOptions { Size = 5 }); Assert.Equal("value", cache.Get("key")); - Assert.Equal(5, cache.Size); + AssertCacheSize(5, cache); cache.Set("key", "value1", new MemoryCacheEntryOptions { Size = 6 }); Assert.Null(cache.Get("key")); - Assert.Equal(0, cache.Size); + AssertCacheSize(0, cache); } [Fact] @@ -251,12 +251,12 @@ public async Task AddingReplacementWhenTotalSizeExceedsCapacityDoesNotUpdateRemo State = null }); - Assert.Equal(0, cache.Size); + AssertCacheSize(0, cache); cache.Set("key", "value", entryOptions); Assert.Equal("value", cache.Get("key")); - Assert.Equal(6, cache.Size); + AssertCacheSize(6, cache); cache.Set("key", "value1", new MemoryCacheEntryOptions { Size = 5 }); @@ -264,7 +264,7 @@ public async Task AddingReplacementWhenTotalSizeExceedsCapacityDoesNotUpdateRemo Assert.True(await sem.WaitAsync(TimeSpan.FromSeconds(10))); Assert.Null(cache.Get("key")); - Assert.Equal(0, cache.Size); + AssertCacheSize(0, cache); } [Fact] @@ -276,17 +276,18 @@ public void AddingReplacementExceedsCapacityRemovesOldEntry() CompactionPercentage = 0.5 }); - Assert.Equal(0, cache.Size); + AssertCacheSize(0, cache); cache.Set("key", "value", new MemoryCacheEntryOptions { Size = 6 }); Assert.Equal("value", cache.Get("key")); - Assert.Equal(6, cache.Size); + + AssertCacheSize(6, cache); cache.Set("key", "value1", new MemoryCacheEntryOptions { Size = 11 }); Assert.Null(cache.Get("key")); - Assert.Equal(0, cache.Size); + AssertCacheSize(0, cache); // addition was rejected due to size, and previous item with the same key removed } [Fact] @@ -296,11 +297,11 @@ public void RemovingEntryDecreasesCacheSize() cache.Set("key", "value", new MemoryCacheEntryOptions { Size = 5 }); - Assert.Equal(5, cache.Size); + AssertCacheSize(5, cache); cache.Remove("key"); - Assert.Equal(0, cache.Size); + AssertCacheSize(0, cache); } [Fact] @@ -324,7 +325,7 @@ public async Task ExpiringEntryDecreasesCacheSize() cache.Set("key", "value", entryOptions); - Assert.Equal(5, cache.Size); + AssertCacheSize(5, cache); // Expire entry changeToken.Fire(); @@ -335,7 +336,7 @@ public async Task ExpiringEntryDecreasesCacheSize() // Wait for compaction to complete Assert.True(await sem.WaitAsync(TimeSpan.FromSeconds(10))); - Assert.Equal(0, cache.Size); + AssertCacheSize(0, cache); } [Fact] @@ -351,9 +352,9 @@ public void TryingToAddExpiredEntryDoesNotIncreaseCacheSize() }; cache.Set("key", "value", entryOptions); - + Assert.Null(cache.Get("key")); - Assert.Equal(0, cache.Size); + AssertCacheSize(0, cache); } [Fact] @@ -368,9 +369,9 @@ public void TryingToAddEntryWithExpiredTokenDoesNotIncreaseCacheSize() }; cache.Set("key", "value", entryOptions); - + Assert.Null(cache.Get("key")); - Assert.Equal(0, cache.Size); + AssertCacheSize(0, cache); } [Fact] @@ -444,14 +445,23 @@ public void NoCompactionWhenNoMaximumEntriesCountSpecified() public void ClearZeroesTheSize() { var cache = new MemoryCache(new MemoryCacheOptions { SizeLimit = 10 }); - Assert.Equal(0, cache.Size); + AssertCacheSize(0, cache); cache.Set("key", "value", new MemoryCacheEntryOptions { Size = 5 }); - Assert.Equal(5, cache.Size); + AssertCacheSize(5, cache); cache.Clear(); - Assert.Equal(0, cache.Size); + AssertCacheSize(0, cache); Assert.Equal(0, cache.Count); } + + internal static void AssertCacheSize(long size, MemoryCache cache) + { + // Size is only eventually consistent, so retry a few times + RetryHelper.Execute(() => + { + Assert.Equal(size, cache.Size); + }, maxAttempts: 12, (iteration) => (int)Math.Pow(2, iteration)); // 2ms, 4ms.. 4096 ms. Not observed to need over 0.5 sec. + } } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs index 5e79dac863dc0e..761fe4c3ab4892 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs @@ -655,7 +655,7 @@ public void OvercapacityPurge_AreThreadSafe() Assert.Equal(TaskStatus.RanToCompletion, task1.Status); Assert.Equal(TaskStatus.RanToCompletion, task2.Status); Assert.Equal(TaskStatus.RanToCompletion, task3.Status); - Assert.Equal(cache.Count, cache.Size); + CapacityTests.AssertCacheSize(cache.Count, cache); Assert.InRange(cache.Count, 0, 10); Assert.False(limitExceeded); } @@ -716,7 +716,7 @@ public void AddAndReplaceEntries_AreThreadSafe() cacheSize += cache.Get(i); } - Assert.Equal(cacheSize, cache.Size); + CapacityTests.AssertCacheSize(cacheSize, cache); Assert.InRange(cache.Count, 0, 20); } From ee4f015fd03e03f76e48f5041f7c4766b9bd5e3b Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Mon, 25 Jul 2022 17:56:26 -0600 Subject: [PATCH 5/7] Comment --- .../Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index 6844186533e040..b9fbf98e63bc9c 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -426,6 +426,10 @@ private void ScanForExpiredItems() } } + /// + /// Returns true if increasing the cache size by the size of entry would + /// cause it to exceed any size limit on the cache, otherwise, returns false. + /// private bool UpdateCacheSizeExceedsCapacity(CacheEntry entry, CoherentState coherentState) { long sizeLimit = _options.SizeLimitValue; From 49eb05adcbd71587626f18f05d7f7b0b16594cf5 Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Mon, 25 Jul 2022 18:06:01 -0600 Subject: [PATCH 6/7] comment --- .../Microsoft.Extensions.Caching.Memory/tests/CapacityTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CapacityTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CapacityTests.cs index 31a92dddeb0e32..8a0ba15ac8fcfd 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CapacityTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CapacityTests.cs @@ -461,7 +461,7 @@ internal static void AssertCacheSize(long size, MemoryCache cache) RetryHelper.Execute(() => { Assert.Equal(size, cache.Size); - }, maxAttempts: 12, (iteration) => (int)Math.Pow(2, iteration)); // 2ms, 4ms.. 4096 ms. Not observed to need over 0.5 sec. + }, maxAttempts: 12, (iteration) => (int)Math.Pow(2, iteration)); // 2ms, 4ms.. 4096 ms. In practice, retries are rarely needed. } } } From 874004d6281513e1f6d2823bd5128b55cef37ce6 Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Mon, 25 Jul 2022 18:22:54 -0600 Subject: [PATCH 7/7] comment --- .../Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index b9fbf98e63bc9c..88b4af2a9e9c8c 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -627,12 +627,12 @@ private static void ValidateCacheKey(object key) /// /// Entries may have various sizes. If a size limit has been set, the cache keeps track of the aggregate of all the entries' sizes /// in order to trigger compaction when the size limit is exceeded. - /// Because the overall size is used only to trigger compaction, it is not necessary to update it atomically with the collection, - /// so long as it is always eventually consistent. So instead of locking around updates to the collection and the overall size, - /// the size is updated after the collection, using an Interlocked operation. + /// + /// For performance reasons, the size is not updated atomically with the collection, but is only made eventually consistent. /// /// When the memory cache is cleared, it replaces the backing collection entirely. This may occur in parallel with operations /// like add, set, remove, and compact which may modify the collection and thus its overall size. + /// /// To keep the overall size eventually consistent, therefore, the collection and the overall size are wrapped in this CoherentState /// object. Individual operations take a local reference to this wrapper object while they work, and make size updates to this object. /// Clearing the cache simply replaces the object, so that any still in progress updates do not affect the overall size value for