From 12b6c3d35662f8f0f35035bf53871ea6c3d7d549 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Cant=C3=BA?= Date: Mon, 24 Jun 2024 17:56:39 -0500 Subject: [PATCH 1/2] Subtract prior entry size when adding entry to cache --- .../src/MemoryCache.cs | 20 ++++++-------- .../tests/CapacityTests.cs | 27 ++++++++++++++++++- 2 files changed, 34 insertions(+), 13 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index ed711c0ffa0e9f..710431f6d5e762 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -153,7 +153,7 @@ internal void SetEntry(CacheEntry entry) coherentState.RemoveEntry(priorEntry, _options); } } - else if (!UpdateCacheSizeExceedsCapacity(entry, coherentState)) + else if (!UpdateCacheSizeExceedsCapacity(entry, priorEntry, coherentState)) { bool entryAdded; if (priorEntry == null) @@ -166,15 +166,7 @@ internal void SetEntry(CacheEntry entry) // Try to update with the new entry if a previous entries exist. entryAdded = coherentState._entries.TryUpdate(entry.Key, entry, priorEntry); - if (entryAdded) - { - if (_options.HasSizeLimit) - { - // The prior entry was removed, decrease the by the prior entry's size - Interlocked.Add(ref coherentState._cacheSize, -priorEntry.Size); - } - } - else + if (!entryAdded) { // The update will fail if the previous entry was removed after retrieval. // Adding the new entry will succeed only if no entry has been added since. @@ -192,7 +184,7 @@ internal void SetEntry(CacheEntry entry) if (_options.HasSizeLimit) { // Entry could not be added, reset cache size - Interlocked.Add(ref coherentState._cacheSize, -entry.Size); + Interlocked.Add(ref coherentState._cacheSize, -entry.Size + (priorEntry?.Size ?? 0)); } entry.SetExpired(EvictionReason.Replaced); entry.InvokeEvictionCallbacks(); @@ -444,7 +436,7 @@ 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) + private bool UpdateCacheSizeExceedsCapacity(CacheEntry entry, CacheEntry? priorEntry, CoherentState coherentState) { long sizeLimit = _options.SizeLimitValue; if (sizeLimit < 0) @@ -456,6 +448,10 @@ private bool UpdateCacheSizeExceedsCapacity(CacheEntry entry, CoherentState cohe for (int i = 0; i < 100; i++) { long newSize = sizeRead + entry.Size; + if (priorEntry != null && entry.Key == priorEntry.Key) + { + newSize -= priorEntry.Size; + } if ((ulong)newSize > (ulong)sizeLimit) { diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CapacityTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CapacityTests.cs index 2be2a66bd60e6b..69177778c3ea2e 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CapacityTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CapacityTests.cs @@ -219,7 +219,7 @@ public void AddingReplacementWhenTotalSizeExceedsCapacityDoesNotUpdateAndRemoves { var cache = new MemoryCache(new MemoryCacheOptions { - SizeLimit = 10, + SizeLimit = 5, CompactionPercentage = 0.5 }); @@ -236,6 +236,31 @@ public void AddingReplacementWhenTotalSizeExceedsCapacityDoesNotUpdateAndRemoves AssertCacheSize(0, cache); } + [Theory] + [InlineData(6)] + [InlineData(5)] + [InlineData(2)] + public void ReplaceOldEntryWithSameSizeOrLessNewEntryAtSizeLimitCapacity(int newValueSize) + { + var cache = new MemoryCache(new MemoryCacheOptions + { + SizeLimit = 6 + }); + + AssertCacheSize(0, cache); + + cache.Set("key", "oldValue", new MemoryCacheEntryOptions { Size = 6 }); + + Assert.Equal("oldValue", cache.Get("key")); + + AssertCacheSize(6, cache); + + cache.Set("key", "newValue", new MemoryCacheEntryOptions { Size = newValueSize }); + + Assert.Equal("newValue", cache.Get("key")); + AssertCacheSize(newValueSize, cache); + } + [Fact] [ActiveIssue("https://github.com/dotnet/runtime/issues/72912")] public async Task AddingReplacementWhenTotalSizeExceedsCapacityDoesNotUpdateRemovesOldEntryAndTriggersCompaction() From 0d690b97e4959cd2a1773d1818c1b6a13f805822 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Cant=C3=BA?= Date: Thu, 27 Jun 2024 11:26:16 -0500 Subject: [PATCH 2/2] Address feedback --- .../Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index 710431f6d5e762..5da4422a00ccfb 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; +using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Runtime.CompilerServices; using System.Threading; @@ -184,7 +185,7 @@ internal void SetEntry(CacheEntry entry) if (_options.HasSizeLimit) { // Entry could not be added, reset cache size - Interlocked.Add(ref coherentState._cacheSize, -entry.Size + (priorEntry?.Size ?? 0)); + Interlocked.Add(ref coherentState._cacheSize, -entry.Size + (priorEntry?.Size).GetValueOrDefault()); } entry.SetExpired(EvictionReason.Replaced); entry.InvokeEvictionCallbacks(); @@ -448,8 +449,9 @@ private bool UpdateCacheSizeExceedsCapacity(CacheEntry entry, CacheEntry? priorE for (int i = 0; i < 100; i++) { long newSize = sizeRead + entry.Size; - if (priorEntry != null && entry.Key == priorEntry.Key) + if (priorEntry != null) { + Debug.Assert(entry.Key == priorEntry.Key); newSize -= priorEntry.Size; }