MemoryCache eviction restructure #280

JunTaoLuo · 2017-03-10T23:34:38Z

#221 Need to review API changes and then continue with additional todos

TODO:

tests
doc comments
perf
samples?

davidfowl · 2017-03-11T18:38:11Z

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

+    public interface IMemoryCacheEvictionStrategy
+    {
+        // TODO: doc comments
+        IEnumerable<CacheEntry> GetEntriesToEvict(IEnumerable<CacheEntry> entries, DateTimeOffset now);


Should this be IList<CacheEntry>? This structure worries me a little because it requires the caller to do a LINQ query (or some equivalent) by default. We should add a micro benchmark to see what the performance overhead of this is.

Yea although this is the most generic collection, it may be a little too limited in terms of operations that are available on it.

Rather than returning a list, maybe just call a method / set a property on a given entry to mark it for eviction. Evict()

Actually, SetExpired pretty much already does that.

I like that.

That would work.

Tratcher

Concerned about the abstraction leaking, but I'll take another look when I have more screens available.

Tratcher · 2017-03-13T19:36:32Z

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

+
+namespace Microsoft.Extensions.Caching.Memory
+{
+    public class DefaultMemoryCacheEvictionStrategy : IMemoryCacheEvictionStrategy


Avoid the word Default in type names

JunTaoLuo · 2017-03-14T23:05:32Z

Any actionable changes to the API before I continue working on this @davidfowl @Tratcher ?

Tratcher · 2017-03-15T02:36:18Z

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

@@ -9,7 +9,7 @@

 namespace Microsoft.Extensions.Caching.Memory
 {
-    internal class CacheEntry : ICacheEntry
+    public class CacheEntry : ICacheEntry


CacheEntry is the wrong view to expose to the eviction algorithm. This view is the one used for creating the entry so Value is settable, all the expirations are settable, AbsoluteExpirationRelativeToNow is relative to entry creation rather than the expiration scan, etc..

There should be a different interface/view exposed for creation vs eviction, even if the same object implements both internally.

I'll give this a try

Tratcher · 2017-03-15T02:43:26Z

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

+    public interface IMemoryCacheEvictionStrategy
+    {
+        // TODO: doc comments
+        IEnumerable<CacheEntry> GetEntriesToEvict(IEnumerable<CacheEntry> entries, DateTimeOffset now);


Rather than returning a list, maybe just call a method / set a property on a given entry to mark it for eviction. Evict()

Tratcher · 2017-03-15T02:45:44Z

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

+    public interface IMemoryCacheEvictionStrategy
+    {
+        // TODO: doc comments
+        IEnumerable<CacheEntry> GetEntriesToEvict(IEnumerable<CacheEntry> entries, DateTimeOffset now);


Actually, SetExpired pretty much already does that.

Tratcher · 2017-03-15T02:49:47Z

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

@@ -148,7 +136,6 @@ public static ICacheEntry SetOptions(this ICacheEntry entry, MemoryCacheEntryOpt
            entry.AbsoluteExpiration = options.AbsoluteExpiration;
            entry.AbsoluteExpirationRelativeToNow = options.AbsoluteExpirationRelativeToNow;
            entry.SlidingExpiration = options.SlidingExpiration;
-            entry.Priority = options.Priority;


Removing Priority is unfortunate but I understand. Do we have another way to add custom metadata for the eviction algorithm without embedding it in the value itself? What if ICacheEntry had a transparent object EvictionMetadata property?

Not sure about this since EvictionMetadata may not make sense when all components share the same memory cache in DI. Would each component then have to know what to set the EvictionMetadata to? Also, what is the overhead of embedding the information in the value itself over having this field (would need to test this in a microbenchmark).

All components share an eviction strategy anyways, and if the metadata were embedded in the value you'd have the same problem of everyone having to know about it.

JunTaoLuo · 2017-03-16T00:32:11Z

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

+
+        private void TimerLoop(object state)
+        {
+            if (EvictionCallback())


Need to have logic to prevent this from being re-entrant.

Tratcher · 2017-03-17T21:30:37Z

Think about the error handling for this new extensibility, especially on the timer thread.

Tratcher · 2017-03-17T21:48:32Z

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

@@ -11,9 +11,9 @@ public class MemoryCacheOptions : IOptions<MemoryCacheOptions>
    {
        public ISystemClock Clock { get; set; }

-        public bool CompactOnMemoryPressure { get; set; } = true;
+        public IMemoryCacheEvictionStrategy EvictionStrategy { get; set; }


Why are these options rather than DI injected?

Tratcher · 2017-03-17T21:50:57Z

src/Microsoft.Extensions.Caching.Memory/Interfaces/IMemoryCacheEvictionTrigger.cs

+    public interface IMemoryCacheEvictionTrigger : IDisposable
+    {
+        // TODO: doc comments
+        Func<bool> EvictionCallback { get; set; }


void Initialize(Func<bool> evictionCallback); or make it a parameter to Resume

Tratcher · 2017-03-17T21:56:34Z

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

        {
            return _isExpired || CheckForExpiredTime(now) || CheckForExpiredTokens();
        }

-        internal void SetExpired(EvictionReason reason)
+        public void SetExpired(EvictionReason reason)


It's a little odd for this to be public. The only EvictionReason that would make sense for the eviction algorithm to set would be Capacity. Expired is the next best reason, but CheckExpired should set that. Maybe make a new method that's just Evict and always uses Capacity as the reason?

Tratcher · 2017-03-23T15:04:16Z

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

@@ -16,9 +13,9 @@ namespace Microsoft.Extensions.Caching.Memory
    /// An implementation of <see cref="IMemoryCache"/> using a dictionary to
    /// store its entries.
    /// </summary>
-    public class MemoryCache : IMemoryCache
+    public partial class MemoryCache : IMemoryCache


JunTaoLuo · 2017-03-23T19:08:44Z

Closing this PR, we decided to remove the Gen2 GC trigger and allow users to downcast to use Compact on MemoryCache instead.

JunTaoLuo requested a review from davidfowl March 10, 2017 23:34

dnfclas added the cla-already-signed label Mar 10, 2017

davidfowl reviewed Mar 11, 2017

View reviewed changes

Tratcher reviewed Mar 13, 2017

View reviewed changes

Tratcher suggested changes Mar 15, 2017

View reviewed changes

JunTaoLuo added 11 commits March 15, 2017 14:57

WIP EvictionStrategy based compaction for MemoryCache

8210679

Test eviction with default timer

9be39b1

Undo sln change

618ab06

Remove CacheItemPriority and add sample eviction strategies

76c91a3

Relocate abstraction to Caching.Memory package

479809b

Add default time based trigger

b11dfa0

Remove duplicate expiration scans

9759c5f

typo

8f50c20

Remove redundant options and make DefaultEvictionStrategy virtual

9903c59

Cleanup

3b9b116

Untested WIP

a1b9702

JunTaoLuo force-pushed the johluo/eviction branch from 80931d4 to a1b9702 Compare March 16, 2017 00:15

JunTaoLuo commented Mar 16, 2017

View reviewed changes

JunTaoLuo added 6 commits March 15, 2017 17:41

Non re-entrant timer

7a1f2cd

Add EvictionMetadata

4ec49dc

Store a dictionary of IRetrievedCacheEntry

05c0b69

Rework Trigger

25d4725

Expose as readonly collection

4a4bda7

Simplify trigger

78f2b40

Tratcher reviewed Mar 17, 2017

View reviewed changes

Update

d5629fb

JunTaoLuo added 3 commits March 19, 2017 02:17

Update sample

9cee4ff

Update

f8fc204

Box things nicely

89aa0b8

Tratcher reviewed Mar 23, 2017

View reviewed changes

Poor refactoring

f854820

JunTaoLuo closed this Mar 23, 2017

natemcmaster deleted the johluo/eviction branch November 7, 2018 04:54

JunTaoLuo mentioned this pull request May 15, 2020

Revisit MemoryCache design dotnet/runtime#36556

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MemoryCache eviction restructure #280

MemoryCache eviction restructure #280

JunTaoLuo commented Mar 10, 2017 •

edited

Loading

davidfowl Mar 11, 2017

JunTaoLuo Mar 13, 2017

Tratcher Mar 15, 2017

Tratcher Mar 15, 2017

davidfowl Mar 15, 2017

JunTaoLuo Mar 15, 2017

Tratcher left a comment

Tratcher Mar 13, 2017

JunTaoLuo commented Mar 14, 2017

Tratcher Mar 15, 2017

JunTaoLuo Mar 15, 2017

Tratcher Mar 15, 2017

Tratcher Mar 15, 2017

Tratcher Mar 15, 2017

JunTaoLuo Mar 15, 2017

Tratcher Mar 15, 2017

Tratcher Mar 17, 2017

JunTaoLuo Mar 16, 2017

Tratcher commented Mar 17, 2017

Tratcher Mar 17, 2017

Tratcher Mar 17, 2017

Tratcher Mar 17, 2017

Tratcher Mar 23, 2017

JunTaoLuo commented Mar 23, 2017

MemoryCache eviction restructure #280

MemoryCache eviction restructure #280

Conversation

JunTaoLuo commented Mar 10, 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

Tratcher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunTaoLuo commented Mar 14, 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

Tratcher commented Mar 17, 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

JunTaoLuo commented Mar 23, 2017

JunTaoLuo commented Mar 10, 2017 •

edited

Loading