Add size based eviction to MemoryCache #332

JunTaoLuo · 2017-07-04T23:38:47Z

Addresses #326

TODO:

JunTaoLuo · 2017-07-04T23:41:11Z

src/Microsoft.Extensions.Caching.Memory/MemoryCacheOptions.cs

@@ -13,6 +13,12 @@ public class MemoryCacheOptions : IOptions<MemoryCacheOptions>

        public TimeSpan ExpirationScanFrequency { get; set; } = TimeSpan.FromMinutes(1);

+        public bool EnforceSizeLimit { get; set; } = false;
+
+        public long HighWatermark { get; set; }


Potentially extracted these options to a separate class/policy.

Naming, docs.
Make it nullable rather than having a separate EnforceSizeLimit bool?

Tratcher · 2017-07-05T16:20:05Z

src/Microsoft.Extensions.Caching.Memory/MemoryCacheOptions.cs

@@ -13,6 +13,12 @@ public class MemoryCacheOptions : IOptions<MemoryCacheOptions>

        public TimeSpan ExpirationScanFrequency { get; set; } = TimeSpan.FromMinutes(1);

+        public bool EnforceSizeLimit { get; set; } = false;
+
+        public long HighWatermark { get; set; }


Naming, docs.
Make it nullable rather than having a separate EnforceSizeLimit bool?

Tratcher · 2017-07-05T16:38:51Z

src/Microsoft.Extensions.Caching.Memory/MemoryCache.cs

+            {
+                if (_options.HighWatermark <= 0)
+                {
+                    throw new ArgumentOutOfRangeException(nameof(_options.HighWatermark), _options.HighWatermark, $"{nameof(_options.HighWatermark)} must be positive.");


Move the validation to the options class.

Tratcher · 2017-07-05T16:39:29Z

src/Microsoft.Extensions.Caching.Memory/MemoryCache.cs

+                }
+                if (entry.Size.Value < 0)
+                {
+                    throw new ArgumentOutOfRangeException(nameof(entry.Size.Value), entry.Size.Value, $"{nameof(entry.Size.Value)} must be non-negative.");


Move this validation to the property

Tratcher · 2017-07-05T16:43:23Z

src/Microsoft.Extensions.Caching.Memory/MemoryCache.cs

@@ -90,7 +115,19 @@ private void SetEntry(CacheEntry entry)
                return;
            }

-            var utcNow = _clock.UtcNow;
+            if (_options.EnforceSizeLimit)


Prevent adding the entry if doing so would exceed the cache size limit.

Tratcher · 2017-07-05T16:44:28Z

src/Microsoft.Extensions.Caching.Memory/MemoryCache.cs

+                try
+                {
+                    var currentSize = Interlocked.Read(ref _cacheSize);
+                    while (currentSize > _options.HighWatermark)


No loop. Try again next time.

Tratcher · 2017-07-05T16:48:31Z

src/Microsoft.Extensions.Caching.Memory/MemoryCache.cs

+            Compact(removalSizeTarget, entry => entry.Size.Value);
+        }
+
+        private void Compact(long removalSizeTarget, Func<CacheEntry, long> computeEntrySize)


Let's discuss

Tratcher · 2017-07-05T16:49:51Z

src/Microsoft.Extensions.Caching.Memory/MemoryCacheServiceCollectionExtensions.cs

+            var memoryCacheOptions = new MemoryCacheOptions();
+            setupAction(memoryCacheOptions);
+
+            services.TryAddSingleton<IDistributedCache>(new MemoryDistributedCache(new MemoryCache(memoryCacheOptions)));


#329 ?
I don't think this was the fix they had in mind.

Tratcher · 2017-07-05T19:28:19Z

MemoryDistributedCache should set sizes by default. This will enable Session to work without changes.

JunTaoLuo · 2017-07-05T20:36:22Z

Caching.sln

@@ -243,4 +243,7 @@ Global
 		{17E332EB-D18D-4BF5-BCA5-989E36C78B79} = {459E1593-2C11-42CB-AD17-F7597E69E5D2}
 		{ADF83AC7-3776-4E62-A222-C6979C32DD2D} = {9E78AA8E-7870-46DE-A49F-856F5A0A9166}
 	EndGlobalSection
+	GlobalSection(ExtensibilityGlobals) = postSolution


Strange, not sure why vs added this.

JunTaoLuo · 2017-07-06T03:50:10Z

src/Microsoft.Extensions.Caching.Memory/MemoryCache.cs

+                            // We have only added the difference between the new size and the old size.
+                            // Since the old one was removed, we need to add the old entry's size back
+                            // to ensure the total size of the new entry is accounted for.
+                            Interlocked.Add(ref _cacheSize, priorEntry.Size.Value);


This could lead to a situation where the capacity is exceeded once SetEntry is complete. Will look for another way to accomplish this.

Tratcher · 2017-07-06T03:55:10Z

src/Microsoft.Extensions.Caching.Abstractions/CacheEntryExtensions.cs

+            this ICacheEntry entry,
+            long size)
+        {
+            if (size <= 0)


Allow 0? E.g. DistributedCache allows byte array lengths of 0, just not null.

Tratcher · 2017-07-06T03:59:29Z

src/Microsoft.Extensions.Caching.Memory/MemoryCache.cs

+                    Interlocked.Add(ref _cacheSize, -sizeUpdate);
+
+                    // Spawn background thread for compaction
+                    ThreadPool.QueueUserWorkItem(new WaitCallback(_ =>


Helper method

I used to have one but this was the only place that was calling it.

Can you remove the closure? Seems like you're not capturing any local state.

Tratcher · 2017-07-06T04:06:16Z

src/Microsoft.Extensions.Caching.Memory/MemoryCache.cs

@@ -122,7 +136,9 @@ private void SetEntry(CacheEntry entry)
                priorEntry.SetExpired(EvictionReason.Replaced);


Why not deduct the size immediately? Then you can deal with just the new size.

I keep the size operations beside the actual calls to remove from the underlying concurrent dictionary. In this case, that's the ScanForExpiredItems call. I think we decided not to remove the entry right away to ensure the value can never be null on replacement hence the awkward logic here. I don't think it's safe to deduct the size here.

davidfowl · 2017-07-06T18:23:07Z

src/Microsoft.Extensions.Caching.Memory/MemoryCache.cs

+        private void TriggerOvercapacityCompaction()
+        {
+            // Spawn background thread for compaction
+            ThreadPool.QueueUserWorkItem(new WaitCallback(_ =>


Remove the new WaitCallback.

davidfowl · 2017-07-06T18:24:02Z

src/Microsoft.Extensions.Caching.Memory/MemoryCacheOptions.cs

        public ISystemClock Clock { get; set; }

        public TimeSpan ExpirationScanFrequency { get; set; } = TimeSpan.FromMinutes(1);

+        public long? SizeLimit


I don't want to do a back and forth but forcing users to calculate a percentage seems bizarre.

Let's talk about this in person

Calculate a percentage? What are their inputs? "If my cache is over capacity I need to purge some data. How much data do I purge?" I'll argue that they'll be more likely to start with a percentage than a fixed value, "free up 5% capacity", which is much simpler to reason about than to say "how big is my total cache? 1gb? Free up 50mb".

muratg · 2017-07-06T18:26:43Z

src/Microsoft.Extensions.Caching.Abstractions/MemoryCacheEntryExtensions.cs

+            this MemoryCacheEntryOptions options,
+            long size)
+        {
+            if (size < 0)


Does size==0 signify anything? Also, is this "Size in bytes"?

We don't want to impose what size means. When we use this in response caching, it will be bytes but in theory you can have a memory cache where all the sizes are 1 and in that case size means count.

Size of zero means you can add this entry even when the cache is full.

Tratcher · 2017-07-06T18:26:22Z

src/Microsoft.Extensions.Caching.Memory/MemoryCache.cs

@@ -90,7 +96,15 @@ private void SetEntry(CacheEntry entry)
                return;
            }

-            var utcNow = _clock.UtcNow;
+            if (_options.SizeLimit != null)


.HasValue everywhere

Tratcher · 2017-07-06T18:34:21Z

src/Microsoft.Extensions.Caching.Memory/MemoryCache.cs

+            ThreadPool.QueueUserWorkItem(new WaitCallback(_ =>
+            {
+                var currentSize = Interlocked.Read(ref _cacheSize);
+                if (currentSize > _options.SizeLimit * (1 - _options.RemovalPercentageOnOvercapacityCompaction))


save your calculation in a local variable to avoid repetition.

Tratcher · 2017-07-06T18:39:23Z

src/Microsoft.Extensions.Caching.Memory/MemoryCache.cs

+            Compact(removalCountTarget, _ => 1);
+        }
+
+        private void Compact(long removalSizeTarget)


this helper method isn't doing anything, do you need it?

Tratcher · 2017-07-06T18:47:30Z

src/Microsoft.Extensions.Caching.Memory/MemoryCacheOptions.cs

        public ISystemClock Clock { get; set; }

        public TimeSpan ExpirationScanFrequency { get; set; } = TimeSpan.FromMinutes(1);

+        public long? SizeLimit


Calculate a percentage? What are their inputs? "If my cache is over capacity I need to purge some data. How much data do I purge?" I'll argue that they'll be more likely to start with a percentage than a fixed value, "free up 5% capacity", which is much simpler to reason about than to say "how big is my total cache? 1gb? Free up 50mb".

JunTaoLuo · 2017-07-06T22:26:55Z

src/Microsoft.Extensions.Caching.Abstractions/CacheEntryExtensions.cs

+            this ICacheEntry entry,
+            long size)
+        {
+            if (size < 0)


Add some tests for these checks

Tratcher · 2017-07-06T22:54:42Z

src/Microsoft.Extensions.Caching.Memory/MemoryCache.cs

@@ -162,6 +188,17 @@ private void SetEntry(CacheEntry entry)
            }
            else
            {
+                if (_options.SizeLimit.HasValue && updatedCacheSize > _options.SizeLimit)


isn't this just exceedsCapacity?

Tratcher · 2017-07-06T22:56:39Z

src/Microsoft.Extensions.Caching.Memory/MemoryCacheOptions.cs

+            }
+        }
+
+        public double RemovalPercentageOnOvercapacityCompaction


CompactionPercentage?

Tratcher · 2017-07-06T23:16:05Z

src/Microsoft.Extensions.Caching.Memory/MemoryCacheOptions.cs

+            get => _sizeLimit;
+            set
+            {
+                if (value <= 0)


allow 0 for test scenarios. e.g. temporarily reduce the cache limit to 0 to debug an issue masked (or caused) by caching.

Tratcher · 2017-07-07T04:00:44Z

src/Microsoft.Extensions.Caching.Abstractions/CacheEntryExtensions.cs

@@ -134,6 +134,24 @@ public static class CacheEntryExtensions
        }

        /// <summary>
+        /// Sets the size of the cache entry.


the cache entry Value.
The thing being cached, not the infrastructure around it.

Tratcher · 2017-07-07T04:10:25Z

src/Microsoft.Extensions.Caching.Memory/MemoryCache.cs

@@ -272,6 +321,53 @@ private static void ScanForExpiredItems(MemoryCache cache)
            }
        }

+        private bool UpdateCacheSizeExceedsCapacity(CacheEntry entry, out bool cacheSizeUpdated)


Can you eliminate cacheSizeUpdated? e.g. if it exceed capacity, immediately subtract it again. Then the only info you need out of this method is the return bool.

Possible, I'll take a closer look in the morning.

Tratcher · 2017-07-07T04:20:04Z

src/Microsoft.Extensions.Caching.Memory/MemoryCache.cs

+            }
+
+            var newSize = 0L;
+            while(true)


finite retries? e.g. if it fails 100 times then give up and return that you're over capacity. Under stress you may get a bunch of threads in this loop competing with eachother and there's no gurantee how long they'll be spinning. Some poor threads could be stuck here forever burning all the CPU on your machine (unlikely but possible). MemoryCache needs to be fast and determanistic, and that includes failing fast if necessary.

JunTaoLuo · 2017-07-07T07:36:21Z

test/Microsoft.Extensions.Caching.Memory.Tests/CapacityTests.cs

+            testClock.Add(TimeSpan.FromSeconds(10));
+
+            // Wait for compaction to complete
+            Thread.Sleep(TimeSpan.FromSeconds(4));


Increased these waits significantly to ensure tests pass on appveyor and travis. This is a rather poor approach and I'll see if there are better ways to ensure the consistency of these tests.

Tratcher · 2017-07-07T21:35:02Z

src/Microsoft.Extensions.Caching.Memory/MemoryCache.cs

+                var sizeRead = Interlocked.Read(ref _cacheSize);
+                newSize = sizeRead + entry.Size.Value;
+
+                if (newSize < 0)


Move newSize > _options.SizeLimit here and get rid of the decrement.

Tratcher · 2017-07-07T22:21:54Z

test/Microsoft.Extensions.Caching.Memory.Tests/CapacityTests.cs

+        {
+            var cache = new MemoryCache(new MemoryCacheOptions
+            {
+                ExpirationScanFrequency = TimeSpan.Zero,


dnfclas added the cla-already-signed label Jul 4, 2017

JunTaoLuo commented Jul 4, 2017

View reviewed changes

JunTaoLuo mentioned this pull request Jul 5, 2017

Add option to MemoryCacheOptions to specify maximum cache entries count #327

Closed

Tratcher suggested changes Jul 5, 2017

View reviewed changes

JunTaoLuo force-pushed the johluo/max-size branch from 630ae34 to ff010f1 Compare July 5, 2017 20:35

JunTaoLuo commented Jul 5, 2017

View reviewed changes

JunTaoLuo commented Jul 6, 2017

View reviewed changes

Tratcher reviewed Jul 6, 2017

View reviewed changes

davidfowl reviewed Jul 6, 2017

View reviewed changes

muratg reviewed Jul 6, 2017

View reviewed changes

JunTaoLuo changed the title ~~WIP Add size based eviction to MemoryCache~~ Add size based eviction to MemoryCache Jul 6, 2017

Tratcher reviewed Jul 6, 2017

View reviewed changes

JunTaoLuo force-pushed the johluo/max-size branch from 293b6c8 to b411284 Compare July 6, 2017 19:42

JunTaoLuo commented Jul 6, 2017

View reviewed changes

Tratcher reviewed Jul 6, 2017

View reviewed changes

Tratcher reviewed Jul 7, 2017

View reviewed changes

JunTaoLuo commented Jul 7, 2017

View reviewed changes

JunTaoLuo force-pushed the johluo/max-size branch from 537c195 to bb2a3ad Compare July 7, 2017 19:18

Tratcher reviewed Jul 7, 2017

View reviewed changes

Add size based eviction to MemoryCache

45d42c2

JunTaoLuo force-pushed the johluo/max-size branch from 2fcf8c2 to 45d42c2 Compare July 7, 2017 22:59

Tratcher approved these changes Jul 7, 2017

View reviewed changes

JunTaoLuo mentioned this pull request Jul 7, 2017

Add MemoryDistributedCacheOptions and use it to configure MemoryCache used by MemoryDistributedCache #336

Closed

JunTaoLuo merged commit 45d42c2 into dev Jul 8, 2017

JunTaoLuo deleted the johluo/max-size branch July 8, 2017 04:21

		@@ -122,7 +136,9 @@ private void SetEntry(CacheEntry entry)
		priorEntry.SetExpired(EvictionReason.Replaced);

Add size based eviction to MemoryCache #332

Add size based eviction to MemoryCache #332

Conversation

JunTaoLuo commented Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher commented Jul 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunTaoLuo commented Jul 4, 2017 •

edited

Loading

Tratcher Jul 7, 2017 •

edited

Loading