Reduce CacheEntry size (#45962)
authorAdam Sitnik <adam.sitnik@gmail.com>
Thu, 7 Jan 2021 19:57:33 +0000 (20:57 +0100)
committerGitHub <noreply@github.com>
Thu, 7 Jan 2021 19:57:33 +0000 (20:57 +0100)
* reduce the size of CacheEntry instance by 2xsizeof(int)

* remove _lock field and use "this" for locking instead

* move _expirationTokenRegistrations, _postEvictionCallbacks and _expirationTokens to separate type to reduce the typical cache entry size

* apply code review suggestion: don't use Enum.HasFlag

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
* Apply suggestions from code review

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Co-authored-by: Stephen Toub <stoub@microsoft.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Co-authored-by: Stephen Toub <stoub@microsoft.com>
src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs [new file with mode: 0644]
src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs [new file with mode: 0644]
src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs
src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs

diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs
new file mode 100644 (file)
index 0000000..77aac7b
--- /dev/null
@@ -0,0 +1,61 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+
+namespace Microsoft.Extensions.Caching.Memory
+{
+    internal partial class CacheEntry
+    {
+        // this type exists just to reduce CacheEntry size by replacing many enum & boolean fields with one of a size of Int32
+        private struct CacheEntryState
+        {
+            private byte _flags;
+            private byte _evictionReason;
+            private byte _priority;
+
+            internal CacheEntryState(CacheItemPriority priority) : this() => _priority = (byte)priority;
+
+            internal bool IsDisposed
+            {
+                get => ((Flags)_flags & Flags.IsDisposed) != 0;
+                set => SetFlag(Flags.IsDisposed, value);
+            }
+
+            internal bool IsExpired
+            {
+                get => ((Flags)_flags & Flags.IsExpired) != 0;
+                set => SetFlag(Flags.IsExpired, value);
+            }
+
+            internal bool IsValueSet
+            {
+                get => ((Flags)_flags & Flags.IsValueSet) != 0;
+                set => SetFlag(Flags.IsValueSet, value);
+            }
+
+            internal EvictionReason EvictionReason
+            {
+                get => (EvictionReason)_evictionReason;
+                set => _evictionReason = (byte)value;
+            }
+
+            internal CacheItemPriority Priority
+            {
+                get => (CacheItemPriority)_priority;
+                set => _priority = (byte)value;
+            }
+
+            private void SetFlag(Flags option, bool value) => _flags = (byte)(value ? (_flags | (byte)option) : (_flags & ~(byte)option));
+
+            [Flags]
+            private enum Flags : byte
+            {
+                Default = 0,
+                IsValueSet = 1 << 0,
+                IsExpired = 1 << 1,
+                IsDisposed = 1 << 2,
+            }
+        }
+    }
+}
diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs
new file mode 100644 (file)
index 0000000..5800a40
--- /dev/null
@@ -0,0 +1,139 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Collections.Generic;
+using System.Threading;
+using System.Threading.Tasks;
+using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Primitives;
+
+namespace Microsoft.Extensions.Caching.Memory
+{
+    internal partial class CacheEntry
+    {
+        // this type exists just to reduce average CacheEntry size
+        // which typically is not using expiration tokens or callbacks
+        private sealed class CacheEntryTokens
+        {
+            private List<IChangeToken> _expirationTokens;
+            private List<IDisposable> _expirationTokenRegistrations;
+            private List<PostEvictionCallbackRegistration> _postEvictionCallbacks; // this is not really related to tokens, but was moved here to shrink typical CacheEntry size
+
+            internal List<IChangeToken> ExpirationTokens => _expirationTokens ??= new List<IChangeToken>();
+            internal List<PostEvictionCallbackRegistration> PostEvictionCallbacks => _postEvictionCallbacks ??= new List<PostEvictionCallbackRegistration>();
+
+            internal void AttachTokens()
+            {
+                if (_expirationTokens != null)
+                {
+                    lock (this)
+                    {
+                        for (int i = 0; i < _expirationTokens.Count; i++)
+                        {
+                            IChangeToken expirationToken = _expirationTokens[i];
+                            if (expirationToken.ActiveChangeCallbacks)
+                            {
+                                _expirationTokenRegistrations ??= new List<IDisposable>(1);
+                                IDisposable registration = expirationToken.RegisterChangeCallback(ExpirationCallback, this);
+                                _expirationTokenRegistrations.Add(registration);
+                            }
+                        }
+                    }
+                }
+            }
+
+            internal bool CheckForExpiredTokens(CacheEntry cacheEntry)
+            {
+                if (_expirationTokens != null)
+                {
+                    for (int i = 0; i < _expirationTokens.Count; i++)
+                    {
+                        IChangeToken expiredToken = _expirationTokens[i];
+                        if (expiredToken.HasChanged)
+                        {
+                            cacheEntry.SetExpired(EvictionReason.TokenExpired);
+                            return true;
+                        }
+                    }
+                }
+                return false;
+            }
+
+            internal bool CanPropagateTokens() => _expirationTokens != null;
+
+            internal void PropagateTokens(CacheEntry parentEntry)
+            {
+                if (_expirationTokens != null)
+                {
+                    lock (this)
+                    {
+                        lock (parentEntry.GetOrCreateTokens())
+                        {
+                            foreach (IChangeToken expirationToken in _expirationTokens)
+                            {
+                                parentEntry.AddExpirationToken(expirationToken);
+                            }
+                        }
+                    }
+                }
+            }
+
+            internal void DetachTokens()
+            {
+                // _expirationTokenRegistrations is not checked for null, because AttachTokens might initialize it under lock
+                // instead we are checking for _expirationTokens, because if they are not null, then _expirationTokenRegistrations might also be not null
+                if (_expirationTokens != null)
+                {
+                    lock (this)
+                    {
+                        List<IDisposable> registrations = _expirationTokenRegistrations;
+                        if (registrations != null)
+                        {
+                            _expirationTokenRegistrations = null;
+                            for (int i = 0; i < registrations.Count; i++)
+                            {
+                                IDisposable registration = registrations[i];
+                                registration.Dispose();
+                            }
+                        }
+                    }
+                }
+            }
+
+            internal void InvokeEvictionCallbacks(CacheEntry cacheEntry)
+            {
+                if (_postEvictionCallbacks != null)
+                {
+                    Task.Factory.StartNew(state => InvokeCallbacks((CacheEntry)state), cacheEntry,
+                        CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default);
+                }
+            }
+
+            private static void InvokeCallbacks(CacheEntry entry)
+            {
+                List<PostEvictionCallbackRegistration> callbackRegistrations = Interlocked.Exchange(ref entry._tokens._postEvictionCallbacks, null);
+
+                if (callbackRegistrations == null)
+                {
+                    return;
+                }
+
+                for (int i = 0; i < callbackRegistrations.Count; i++)
+                {
+                    PostEvictionCallbackRegistration registration = callbackRegistrations[i];
+
+                    try
+                    {
+                        registration.EvictionCallback?.Invoke(entry.Key, entry.Value, entry.EvictionReason, registration.State);
+                    }
+                    catch (Exception e)
+                    {
+                        // This will be invoked on a background thread, don't let it throw.
+                        entry._cache._logger.LogError(e, "EvictionCallback invoked failed");
+                    }
+                }
+            }
+        }
+    }
+}
index 6f91cb2..606f0ad 100644 (file)
@@ -6,33 +6,30 @@ using System.Collections.Generic;
 using System.Runtime.CompilerServices;
 using System.Threading;
 using System.Threading.Tasks;
-using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Primitives;
 
 namespace Microsoft.Extensions.Caching.Memory
 {
-    internal class CacheEntry : ICacheEntry
+    internal partial class CacheEntry : ICacheEntry
     {
         private static readonly Action<object> ExpirationCallback = ExpirationTokensExpired;
 
-        private readonly object _lock = new object();
         private readonly MemoryCache _cache;
 
-        private IList<IDisposable> _expirationTokenRegistrations;
-        private IList<PostEvictionCallbackRegistration> _postEvictionCallbacks;
-        private IList<IChangeToken> _expirationTokens;
+        private CacheEntryTokens _tokens; // might be null if user is not using the tokens or callbacks
         private TimeSpan? _absoluteExpirationRelativeToNow;
         private TimeSpan? _slidingExpiration;
         private long? _size;
         private CacheEntry _previous; // this field is not null only before the entry is added to the cache
         private object _value;
-        private int _state; // actually a [Flag] enum called "State"
+        private CacheEntryState _state;
 
         internal CacheEntry(object key, MemoryCache memoryCache)
         {
             Key = key ?? throw new ArgumentNullException(nameof(key));
             _cache = memoryCache ?? throw new ArgumentNullException(nameof(memoryCache));
             _previous = CacheEntryHelper.EnterScope(this);
+            _state = new CacheEntryState(CacheItemPriority.Normal);
         }
 
         /// <summary>
@@ -87,18 +84,18 @@ namespace Microsoft.Extensions.Caching.Memory
         /// <summary>
         /// Gets the <see cref="IChangeToken"/> instances which cause the cache entry to expire.
         /// </summary>
-        public IList<IChangeToken> ExpirationTokens => _expirationTokens ??= new List<IChangeToken>();
+        public IList<IChangeToken> ExpirationTokens => GetOrCreateTokens().ExpirationTokens;
 
         /// <summary>
         /// Gets or sets the callbacks will be fired after the cache entry is evicted from the cache.
         /// </summary>
-        public IList<PostEvictionCallbackRegistration> PostEvictionCallbacks => _postEvictionCallbacks ??= new List<PostEvictionCallbackRegistration>();
+        public IList<PostEvictionCallbackRegistration> PostEvictionCallbacks => GetOrCreateTokens().PostEvictionCallbacks;
 
         /// <summary>
         /// Gets or sets the priority for keeping the cache entry in the cache during a
         /// memory pressure triggered cleanup. The default is <see cref="CacheItemPriority.Normal"/>.
         /// </summary>
-        public CacheItemPriority Priority { get; set; } = CacheItemPriority.Normal;
+        public CacheItemPriority Priority { get => _state.Priority; set => _state.Priority = value; }
 
         /// <summary>
         /// Gets or sets the size of the cache entry value.
@@ -125,30 +122,24 @@ namespace Microsoft.Extensions.Caching.Memory
             set
             {
                 _value = value;
-                IsValueSet = true;
+                _state.IsValueSet = true;
             }
         }
 
         internal DateTimeOffset LastAccessed { get; set; }
 
-        internal EvictionReason EvictionReason { get; private set; }
-
-        private bool IsDisposed { get => ((State)_state).HasFlag(State.IsDisposed); set => Set(State.IsDisposed, value); }
-
-        private bool IsExpired { get => ((State)_state).HasFlag(State.IsExpired); set => Set(State.IsExpired, value); }
-
-        private bool IsValueSet { get => ((State)_state).HasFlag(State.IsValueSet); set => Set(State.IsValueSet, value); }
+        internal EvictionReason EvictionReason { get => _state.EvictionReason; private set => _state.EvictionReason = value; }
 
         public void Dispose()
         {
-            if (!IsDisposed)
+            if (!_state.IsDisposed)
             {
-                IsDisposed = true;
+                _state.IsDisposed = true;
 
                 // Don't commit or propagate options if the CacheEntry Value was never set.
                 // We assume an exception occurred causing the caller to not set the Value successfully,
                 // so don't use this entry.
-                if (IsValueSet)
+                if (_state.IsValueSet)
                 {
                     _cache.SetEntry(this);
 
@@ -163,8 +154,11 @@ namespace Microsoft.Extensions.Caching.Memory
             }
         }
 
-        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        internal bool CheckExpired(in DateTimeOffset now) => IsExpired || CheckForExpiredTime(now) || CheckForExpiredTokens();
+        [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling
+        internal bool CheckExpired(in DateTimeOffset now)
+            => _state.IsExpired
+                || CheckForExpiredTime(now)
+                || (_tokens != null && _tokens.CheckForExpiredTokens(this));
 
         internal void SetExpired(EvictionReason reason)
         {
@@ -172,10 +166,11 @@ namespace Microsoft.Extensions.Caching.Memory
             {
                 EvictionReason = reason;
             }
-            IsExpired = true;
-            DetachTokens();
+            _state.IsExpired = true;
+            _tokens?.DetachTokens();
         }
 
+        [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling
         private bool CheckForExpiredTime(in DateTimeOffset now)
         {
             if (!AbsoluteExpiration.HasValue && !_slidingExpiration.HasValue)
@@ -204,52 +199,7 @@ namespace Microsoft.Extensions.Caching.Memory
             }
         }
 
-        private bool CheckForExpiredTokens()
-        {
-            if (_expirationTokens == null)
-            {
-                return false;
-            }
-
-            return CheckTokens();
-
-            bool CheckTokens()
-            {
-                for (int i = 0; i < _expirationTokens.Count; i++)
-                {
-                    IChangeToken expiredToken = _expirationTokens[i];
-                    if (expiredToken.HasChanged)
-                    {
-                        SetExpired(EvictionReason.TokenExpired);
-                        return true;
-                    }
-                }
-                return false;
-            }
-        }
-
-        internal void AttachTokens()
-        {
-            if (_expirationTokens != null)
-            {
-                lock (_lock)
-                {
-                    for (int i = 0; i < _expirationTokens.Count; i++)
-                    {
-                        IChangeToken expirationToken = _expirationTokens[i];
-                        if (expirationToken.ActiveChangeCallbacks)
-                        {
-                            if (_expirationTokenRegistrations == null)
-                            {
-                                _expirationTokenRegistrations = new List<IDisposable>(1);
-                            }
-                            IDisposable registration = expirationToken.RegisterChangeCallback(ExpirationCallback, this);
-                            _expirationTokenRegistrations.Add(registration);
-                        }
-                    }
-                }
-            }
-        }
+        internal void AttachTokens() => _tokens?.AttachTokens();
 
         private static void ExpirationTokensExpired(object obj)
         {
@@ -262,60 +212,11 @@ namespace Microsoft.Extensions.Caching.Memory
             }, obj, CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default);
         }
 
-        private void DetachTokens()
-        {
-            // _expirationTokenRegistrations is not checked for null, because AttachTokens might initialize it under lock
-            lock (_lock)
-            {
-                IList<IDisposable> registrations = _expirationTokenRegistrations;
-                if (registrations != null)
-                {
-                    _expirationTokenRegistrations = null;
-                    for (int i = 0; i < registrations.Count; i++)
-                    {
-                        IDisposable registration = registrations[i];
-                        registration.Dispose();
-                    }
-                }
-            }
-        }
-
-        internal void InvokeEvictionCallbacks()
-        {
-            if (_postEvictionCallbacks != null)
-            {
-                Task.Factory.StartNew(state => InvokeCallbacks((CacheEntry)state), this,
-                    CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default);
-            }
-        }
-
-        private static void InvokeCallbacks(CacheEntry entry)
-        {
-            IList<PostEvictionCallbackRegistration> callbackRegistrations = Interlocked.Exchange(ref entry._postEvictionCallbacks, null);
-
-            if (callbackRegistrations == null)
-            {
-                return;
-            }
-
-            for (int i = 0; i < callbackRegistrations.Count; i++)
-            {
-                PostEvictionCallbackRegistration registration = callbackRegistrations[i];
-
-                try
-                {
-                    registration.EvictionCallback?.Invoke(entry.Key, entry.Value, entry.EvictionReason, registration.State);
-                }
-                catch (Exception e)
-                {
-                    // This will be invoked on a background thread, don't let it throw.
-                    entry._cache._logger.LogError(e, "EvictionCallback invoked failed");
-                }
-            }
-        }
+        internal void InvokeEvictionCallbacks() => _tokens?.InvokeEvictionCallbacks(this);
 
         // this simple check very often allows us to avoid expensive call to PropagateOptions(CacheEntryHelper.Current)
-        internal bool CanPropagateOptions() => _expirationTokens != null || AbsoluteExpiration.HasValue;
+        [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling
+        internal bool CanPropagateOptions() => (_tokens != null && _tokens.CanPropagateTokens()) || AbsoluteExpiration.HasValue;
 
         internal void PropagateOptions(CacheEntry parent)
         {
@@ -326,19 +227,7 @@ namespace Microsoft.Extensions.Caching.Memory
 
             // Copy expiration tokens and AbsoluteExpiration to the cache entries hierarchy.
             // We do this regardless of it gets cached because the tokens are associated with the value we'll return.
-            if (_expirationTokens != null)
-            {
-                lock (_lock)
-                {
-                    lock (parent._lock)
-                    {
-                        foreach (IChangeToken expirationToken in _expirationTokens)
-                        {
-                            parent.AddExpirationToken(expirationToken);
-                        }
-                    }
-                }
-            }
+            _tokens?.PropagateTokens(parent);
 
             if (AbsoluteExpiration.HasValue)
             {
@@ -349,24 +238,15 @@ namespace Microsoft.Extensions.Caching.Memory
             }
         }
 
-        private void Set(State option, bool value)
+        private CacheEntryTokens GetOrCreateTokens()
         {
-            int before, after;
-
-            do
+            if (_tokens != null)
             {
-                before = _state;
-                after = value ? (_state | (int)option) : (_state & ~(int)option);
-            } while (Interlocked.CompareExchange(ref _state, after, before) != before);
-        }
+                return _tokens;
+            }
 
-        [Flags]
-        private enum State
-        {
-            Default = 0,
-            IsValueSet = 1 << 0,
-            IsExpired = 1 << 1,
-            IsDisposed = 1 << 2,
+            CacheEntryTokens result = new CacheEntryTokens();
+            return Interlocked.CompareExchange(ref _tokens, result, null) ?? result;
         }
     }
 }
index 3ca46d0..9d34d63 100644 (file)
@@ -482,5 +482,35 @@ namespace Microsoft.Extensions.Caching.Memory
             Assert.Null(cache.Get(key2));
             Assert.Null(cache.Get(key4));
         }
+
+        [Fact]
+        public async Task OnceExpiredIsSetToTrueItRemainsTrue()
+        {
+            var cache = CreateCache();
+            var entry = (CacheEntry)cache.CreateEntry("someKey");
+
+            await Task.WhenAll(
+                Task.Run(() => SetExpiredManyTimes(entry)),
+                Task.Run(() => SetExpiredManyTimes(entry)));
+
+            Assert.True(entry.CheckExpired(DateTimeOffset.UtcNow));
+
+            static void SetExpiredManyTimes(CacheEntry cacheEntry)
+            {
+                var now = DateTimeOffset.UtcNow;
+                for (int i = 0; i < 1_000; i++)
+                {
+                    cacheEntry.SetExpired(EvictionReason.Expired); // modifies CacheEntry._state
+                    Assert.True(cacheEntry.CheckExpired(now));
+                    cacheEntry.Value = cacheEntry; // modifies CacheEntry._state
+                    Assert.True(cacheEntry.CheckExpired(now));
+
+                    cacheEntry.SetExpired(EvictionReason.Expired); // modifies CacheEntry._state
+                    Assert.True(cacheEntry.CheckExpired(now));
+                    cacheEntry.Dispose(); // might modify CacheEntry._state
+                    Assert.True(cacheEntry.CheckExpired(now));
+                }
+            }
+        }
     }
 }