From 227c96dbcc398ec660f218880d494f25130baeb1 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 10 Dec 2020 14:35:15 +0100 Subject: [PATCH 01/19] reduce the size of CacheEntry instance by 2xsizeof(int) --- .../src/CacheEntry.cs | 97 +++++++++++++++---- 1 file changed, 77 insertions(+), 20 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index 6f91cb21f038c..f4c09685d9cab 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Logging; @@ -26,13 +27,14 @@ internal class CacheEntry : ICacheEntry 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 State _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 State(CacheItemPriority.Normal); } /// @@ -98,7 +100,7 @@ public TimeSpan? SlidingExpiration /// Gets or sets the priority for keeping the cache entry in the cache during a /// memory pressure triggered cleanup. The default is . /// - public CacheItemPriority Priority { get; set; } = CacheItemPriority.Normal; + public CacheItemPriority Priority { get => _state.Priority; set => _state.Priority = value; } /// /// Gets or sets the size of the cache entry value. @@ -131,13 +133,13 @@ public object Value internal DateTimeOffset LastAccessed { get; set; } - internal EvictionReason EvictionReason { get; private set; } + internal EvictionReason EvictionReason { get => _state.EvictionReason; private set => _state.EvictionReason = value; } - private bool IsDisposed { get => ((State)_state).HasFlag(State.IsDisposed); set => Set(State.IsDisposed, value); } + private bool IsDisposed { get => _state.IsDisposed; set => _state.IsDisposed = value; } - private bool IsExpired { get => ((State)_state).HasFlag(State.IsExpired); set => Set(State.IsExpired, value); } + private bool IsExpired { get => _state.IsExpired; set => _state.IsExpired = value; } - private bool IsValueSet { get => ((State)_state).HasFlag(State.IsValueSet); set => Set(State.IsValueSet, value); } + private bool IsValueSet { get => _state.IsValueSet; set => _state.IsValueSet = value; } public void Dispose() { @@ -349,24 +351,79 @@ internal void PropagateOptions(CacheEntry parent) } } - private void Set(State option, bool value) + [StructLayout(LayoutKind.Explicit)] + private struct State { - int before, after; + [FieldOffset(0)] + private int _state; - do + [FieldOffset(0)] + private byte _flags; + [FieldOffset(1)] + private byte _evictionReason; + [FieldOffset(2)] + private byte _priority; + [FieldOffset(3)] + private byte _reserved; // for future use + + internal State(CacheItemPriority priority) : this() => _priority = (byte)priority; + + internal bool IsDisposed { get => ((Flags)_flags).HasFlag(Flags.IsDisposed); set => SetFlag(Flags.IsDisposed, value); } + + internal bool IsExpired { get => ((Flags)_flags).HasFlag(Flags.IsExpired); set => SetFlag(Flags.IsExpired, value); } + + internal bool IsValueSet { get => ((Flags)_flags).HasFlag(Flags.IsValueSet); set => SetFlag(Flags.IsValueSet, value); } + + internal EvictionReason EvictionReason { - before = _state; - after = value ? (_state | (int)option) : (_state & ~(int)option); - } while (Interlocked.CompareExchange(ref _state, after, before) != before); - } + get => (EvictionReason)_evictionReason; + set + { + State before, after; + do + { + before = this; + after = this; + after._evictionReason = (byte)value; + } while (Interlocked.CompareExchange(ref _state, after._state, before._state) != before._state); + } + } - [Flags] - private enum State - { - Default = 0, - IsValueSet = 1 << 0, - IsExpired = 1 << 1, - IsDisposed = 1 << 2, + internal CacheItemPriority Priority + { + get => (CacheItemPriority)_priority; + set + { + State before, after; + do + { + before = this; + after = this; + after._priority = (byte)value; + } while (Interlocked.CompareExchange(ref _state, after._state, before._state) != before._state); + } + } + + private void SetFlag(Flags option, bool value) + { + State before, after; + + do + { + before = this; + after = this; + after._flags = (byte)(value ? (after._flags | (byte)option) : (after._flags & ~(byte)option)); + } while (Interlocked.CompareExchange(ref _state, after._state, before._state) != before._state); + } + + [Flags] + private enum Flags : byte + { + Default = 0, + IsValueSet = 1 << 0, + IsExpired = 1 << 1, + IsDisposed = 1 << 2, + } } } } From 5c8b86240c9672dc45caa5eadf6de742a932755b Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 10 Dec 2020 14:44:19 +0100 Subject: [PATCH 02/19] remove _lock field and use "this" for locking instead --- .../src/CacheEntry.cs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index f4c09685d9cab..dcba2f032a816 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -16,7 +16,6 @@ internal class CacheEntry : ICacheEntry { private static readonly Action ExpirationCallback = ExpirationTokensExpired; - private readonly object _lock = new object(); private readonly MemoryCache _cache; private IList _expirationTokenRegistrations; @@ -234,7 +233,7 @@ internal void AttachTokens() { if (_expirationTokens != null) { - lock (_lock) + lock (this) { for (int i = 0; i < _expirationTokens.Count; i++) { @@ -267,7 +266,7 @@ private static void ExpirationTokensExpired(object obj) private void DetachTokens() { // _expirationTokenRegistrations is not checked for null, because AttachTokens might initialize it under lock - lock (_lock) + lock (this) { IList registrations = _expirationTokenRegistrations; if (registrations != null) @@ -330,9 +329,9 @@ internal void PropagateOptions(CacheEntry parent) // 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 (this) { - lock (parent._lock) + lock (parent) { foreach (IChangeToken expirationToken in _expirationTokens) { From 4b529fb2f50c0d1b459b302eded9d6e0dbd1b851 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 10 Dec 2020 18:19:10 +0100 Subject: [PATCH 03/19] move _expirationTokenRegistrations, _postEvictionCallbacks and _expirationTokens to separate type to reduce the typicall cache entry size --- .../src/CacheEntry.cs | 200 ++++++++++-------- 1 file changed, 113 insertions(+), 87 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index dcba2f032a816..fbfa0865bec39 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -18,9 +18,7 @@ internal class CacheEntry : ICacheEntry private readonly MemoryCache _cache; - private IList _expirationTokenRegistrations; - private IList _postEvictionCallbacks; - private IList _expirationTokens; + private Tokens _tokens; // might be null if user is not using the tokens or callbacks private TimeSpan? _absoluteExpirationRelativeToNow; private TimeSpan? _slidingExpiration; private long? _size; @@ -88,12 +86,12 @@ public TimeSpan? SlidingExpiration /// /// Gets the instances which cause the cache entry to expire. /// - public IList ExpirationTokens => _expirationTokens ??= new List(); + public IList ExpirationTokens => GetOrCreateTokens().ExpirationTokens; /// /// Gets or sets the callbacks will be fired after the cache entry is evicted from the cache. /// - public IList PostEvictionCallbacks => _postEvictionCallbacks ??= new List(); + public IList PostEvictionCallbacks => GetOrCreateTokens().PostEvictionCallbacks; /// /// Gets or sets the priority for keeping the cache entry in the cache during a @@ -165,7 +163,10 @@ public void Dispose() } [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal bool CheckExpired(in DateTimeOffset now) => IsExpired || CheckForExpiredTime(now) || CheckForExpiredTokens(); + internal bool CheckExpired(in DateTimeOffset now) + => IsExpired + || CheckForExpiredTime(now) + || (_tokens != null && _tokens.CheckForExpiredTokens(this)); internal void SetExpired(EvictionReason reason) { @@ -174,7 +175,7 @@ internal void SetExpired(EvictionReason reason) EvictionReason = reason; } IsExpired = true; - DetachTokens(); + _tokens?.DetachTokens(); } private bool CheckForExpiredTime(in DateTimeOffset now) @@ -205,52 +206,7 @@ bool FullCheck(in DateTimeOffset offset) } } - 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 (this) - { - for (int i = 0; i < _expirationTokens.Count; i++) - { - IChangeToken expirationToken = _expirationTokens[i]; - if (expirationToken.ActiveChangeCallbacks) - { - if (_expirationTokenRegistrations == null) - { - _expirationTokenRegistrations = new List(1); - } - IDisposable registration = expirationToken.RegisterChangeCallback(ExpirationCallback, this); - _expirationTokenRegistrations.Add(registration); - } - } - } - } - } + internal void AttachTokens() => _tokens?.AttachTokens(); private static void ExpirationTokensExpired(object obj) { @@ -263,27 +219,9 @@ private static void ExpirationTokensExpired(object obj) }, obj, CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default); } - private void DetachTokens() - { - // _expirationTokenRegistrations is not checked for null, because AttachTokens might initialize it under lock - lock (this) - { - IList 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) + if (_tokens?._postEvictionCallbacks != null) { Task.Factory.StartNew(state => InvokeCallbacks((CacheEntry)state), this, CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default); @@ -292,7 +230,7 @@ internal void InvokeEvictionCallbacks() private static void InvokeCallbacks(CacheEntry entry) { - IList callbackRegistrations = Interlocked.Exchange(ref entry._postEvictionCallbacks, null); + IList callbackRegistrations = Interlocked.Exchange(ref entry._tokens._postEvictionCallbacks, null); if (callbackRegistrations == null) { @@ -316,7 +254,7 @@ private static void InvokeCallbacks(CacheEntry entry) } // this simple check very often allows us to avoid expensive call to PropagateOptions(CacheEntryHelper.Current) - internal bool CanPropagateOptions() => _expirationTokens != null || AbsoluteExpiration.HasValue; + internal bool CanPropagateOptions() => _tokens?._expirationTokens != null || AbsoluteExpiration.HasValue; internal void PropagateOptions(CacheEntry parent) { @@ -327,19 +265,7 @@ internal void PropagateOptions(CacheEntry parent) // 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 (this) - { - lock (parent) - { - foreach (IChangeToken expirationToken in _expirationTokens) - { - parent.AddExpirationToken(expirationToken); - } - } - } - } + _tokens?.CopyTokens(parent); if (AbsoluteExpiration.HasValue) { @@ -350,6 +276,17 @@ internal void PropagateOptions(CacheEntry parent) } } + private Tokens GetOrCreateTokens() + { + if (_tokens != null) + { + return _tokens; + } + + Tokens result = new Tokens(); + return Interlocked.CompareExchange(ref _tokens, result, null) ?? result; + } + [StructLayout(LayoutKind.Explicit)] private struct State { @@ -424,5 +361,94 @@ private enum Flags : byte IsDisposed = 1 << 2, } } + + private sealed class Tokens + { + internal List _expirationTokens; + internal List _expirationTokenRegistrations; + internal List _postEvictionCallbacks; // this is not really related to tokens, but was moved here to shrink typicall CacheEntry size + + internal List ExpirationTokens => _expirationTokens ??= new List(); + internal List PostEvictionCallbacks => _postEvictionCallbacks ??= new List(); + + internal void AttachTokens() + { + if (_expirationTokens != null) + { + lock (this) + { + for (int i = 0; i < _expirationTokens.Count; i++) + { + IChangeToken expirationToken = _expirationTokens[i]; + if (expirationToken.ActiveChangeCallbacks) + { + if (_expirationTokenRegistrations == null) + { + _expirationTokenRegistrations = new List(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 void CopyTokens(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) + { + IList registrations = _expirationTokenRegistrations; + if (registrations != null) + { + _expirationTokenRegistrations = null; + for (int i = 0; i < registrations.Count; i++) + { + IDisposable registration = registrations[i]; + registration.Dispose(); + } + } + } + } + } + } } } From c04da17d7a6c2f11f94094fe33da542acb9489a5 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 10 Dec 2020 18:27:51 +0100 Subject: [PATCH 04/19] move private types into separate files --- .../src/CacheEntry.cs | 167 +----------------- .../src/CacheEntryState.cs | 87 +++++++++ .../src/CacheEntryTokens.cs | 101 +++++++++++ 3 files changed, 189 insertions(+), 166 deletions(-) create mode 100644 src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryState.cs create mode 100644 src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryTokens.cs diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index fbfa0865bec39..914b25cd5b5a5 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -4,7 +4,6 @@ using System; using System.Collections.Generic; using System.Runtime.CompilerServices; -using System.Runtime.InteropServices; using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Logging; @@ -12,7 +11,7 @@ namespace Microsoft.Extensions.Caching.Memory { - internal class CacheEntry : ICacheEntry + internal partial class CacheEntry : ICacheEntry { private static readonly Action ExpirationCallback = ExpirationTokensExpired; @@ -286,169 +285,5 @@ private Tokens GetOrCreateTokens() Tokens result = new Tokens(); return Interlocked.CompareExchange(ref _tokens, result, null) ?? result; } - - [StructLayout(LayoutKind.Explicit)] - private struct State - { - [FieldOffset(0)] - private int _state; - - [FieldOffset(0)] - private byte _flags; - [FieldOffset(1)] - private byte _evictionReason; - [FieldOffset(2)] - private byte _priority; - [FieldOffset(3)] - private byte _reserved; // for future use - - internal State(CacheItemPriority priority) : this() => _priority = (byte)priority; - - internal bool IsDisposed { get => ((Flags)_flags).HasFlag(Flags.IsDisposed); set => SetFlag(Flags.IsDisposed, value); } - - internal bool IsExpired { get => ((Flags)_flags).HasFlag(Flags.IsExpired); set => SetFlag(Flags.IsExpired, value); } - - internal bool IsValueSet { get => ((Flags)_flags).HasFlag(Flags.IsValueSet); set => SetFlag(Flags.IsValueSet, value); } - - internal EvictionReason EvictionReason - { - get => (EvictionReason)_evictionReason; - set - { - State before, after; - do - { - before = this; - after = this; - after._evictionReason = (byte)value; - } while (Interlocked.CompareExchange(ref _state, after._state, before._state) != before._state); - } - } - - internal CacheItemPriority Priority - { - get => (CacheItemPriority)_priority; - set - { - State before, after; - do - { - before = this; - after = this; - after._priority = (byte)value; - } while (Interlocked.CompareExchange(ref _state, after._state, before._state) != before._state); - } - } - - private void SetFlag(Flags option, bool value) - { - State before, after; - - do - { - before = this; - after = this; - after._flags = (byte)(value ? (after._flags | (byte)option) : (after._flags & ~(byte)option)); - } while (Interlocked.CompareExchange(ref _state, after._state, before._state) != before._state); - } - - [Flags] - private enum Flags : byte - { - Default = 0, - IsValueSet = 1 << 0, - IsExpired = 1 << 1, - IsDisposed = 1 << 2, - } - } - - private sealed class Tokens - { - internal List _expirationTokens; - internal List _expirationTokenRegistrations; - internal List _postEvictionCallbacks; // this is not really related to tokens, but was moved here to shrink typicall CacheEntry size - - internal List ExpirationTokens => _expirationTokens ??= new List(); - internal List PostEvictionCallbacks => _postEvictionCallbacks ??= new List(); - - internal void AttachTokens() - { - if (_expirationTokens != null) - { - lock (this) - { - for (int i = 0; i < _expirationTokens.Count; i++) - { - IChangeToken expirationToken = _expirationTokens[i]; - if (expirationToken.ActiveChangeCallbacks) - { - if (_expirationTokenRegistrations == null) - { - _expirationTokenRegistrations = new List(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 void CopyTokens(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) - { - IList registrations = _expirationTokenRegistrations; - if (registrations != null) - { - _expirationTokenRegistrations = null; - for (int i = 0; i < registrations.Count; i++) - { - IDisposable registration = registrations[i]; - registration.Dispose(); - } - } - } - } - } - } } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryState.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryState.cs new file mode 100644 index 0000000000000..4c4c5d1c748ef --- /dev/null +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryState.cs @@ -0,0 +1,87 @@ +// 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.Runtime.InteropServices; +using System.Threading; + +namespace Microsoft.Extensions.Caching.Memory +{ + internal partial class CacheEntry + { + [StructLayout(LayoutKind.Explicit)] + private struct State + { + [FieldOffset(0)] + private int _state; + + [FieldOffset(0)] + private byte _flags; + [FieldOffset(1)] + private byte _evictionReason; + [FieldOffset(2)] + private byte _priority; + [FieldOffset(3)] + private byte _reserved; // for future use + + internal State(CacheItemPriority priority) : this() => _priority = (byte)priority; + + internal bool IsDisposed { get => ((Flags)_flags).HasFlag(Flags.IsDisposed); set => SetFlag(Flags.IsDisposed, value); } + + internal bool IsExpired { get => ((Flags)_flags).HasFlag(Flags.IsExpired); set => SetFlag(Flags.IsExpired, value); } + + internal bool IsValueSet { get => ((Flags)_flags).HasFlag(Flags.IsValueSet); set => SetFlag(Flags.IsValueSet, value); } + + internal EvictionReason EvictionReason + { + get => (EvictionReason)_evictionReason; + set + { + State before, after; + do + { + before = this; + after = this; + after._evictionReason = (byte)value; + } while (Interlocked.CompareExchange(ref _state, after._state, before._state) != before._state); + } + } + + internal CacheItemPriority Priority + { + get => (CacheItemPriority)_priority; + set + { + State before, after; + do + { + before = this; + after = this; + after._priority = (byte)value; + } while (Interlocked.CompareExchange(ref _state, after._state, before._state) != before._state); + } + } + + private void SetFlag(Flags option, bool value) + { + State before, after; + + do + { + before = this; + after = this; + after._flags = (byte)(value ? (after._flags | (byte)option) : (after._flags & ~(byte)option)); + } while (Interlocked.CompareExchange(ref _state, after._state, before._state) != before._state); + } + + [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/CacheEntryTokens.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryTokens.cs new file mode 100644 index 0000000000000..a06d5ca052c14 --- /dev/null +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryTokens.cs @@ -0,0 +1,101 @@ +// 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 Microsoft.Extensions.Primitives; + +namespace Microsoft.Extensions.Caching.Memory +{ + internal partial class CacheEntry + { + private sealed class Tokens + { + internal List _expirationTokens; + internal List _expirationTokenRegistrations; + internal List _postEvictionCallbacks; // this is not really related to tokens, but was moved here to shrink typicall CacheEntry size + + internal List ExpirationTokens => _expirationTokens ??= new List(); + internal List PostEvictionCallbacks => _postEvictionCallbacks ??= new List(); + + internal void AttachTokens() + { + if (_expirationTokens != null) + { + lock (this) + { + for (int i = 0; i < _expirationTokens.Count; i++) + { + IChangeToken expirationToken = _expirationTokens[i]; + if (expirationToken.ActiveChangeCallbacks) + { + if (_expirationTokenRegistrations == null) + { + _expirationTokenRegistrations = new List(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 void CopyTokens(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) + { + IList registrations = _expirationTokenRegistrations; + if (registrations != null) + { + _expirationTokenRegistrations = null; + for (int i = 0; i < registrations.Count; i++) + { + IDisposable registration = registrations[i]; + registration.Dispose(); + } + } + } + } + } + } + } +} From 217ab4382fceefc0b21d9caa4cbe72a027c9c53f Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 10 Dec 2020 18:37:46 +0100 Subject: [PATCH 05/19] rename the types + add comments --- .../src/CacheEntry.cs | 10 +++++----- .../src/CacheEntryState.cs | 12 +++++++----- .../src/CacheEntryTokens.cs | 4 +++- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index 914b25cd5b5a5..1b2b5cae259c8 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -17,20 +17,20 @@ internal partial class CacheEntry : ICacheEntry private readonly MemoryCache _cache; - private Tokens _tokens; // might be null if user is not using the tokens or callbacks + 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 State _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 State(CacheItemPriority.Normal); + _state = new CacheEntryState(CacheItemPriority.Normal); } /// @@ -275,14 +275,14 @@ internal void PropagateOptions(CacheEntry parent) } } - private Tokens GetOrCreateTokens() + private CacheEntryTokens GetOrCreateTokens() { if (_tokens != null) { return _tokens; } - Tokens result = new Tokens(); + CacheEntryTokens result = new CacheEntryTokens(); return Interlocked.CompareExchange(ref _tokens, result, null) ?? result; } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryState.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryState.cs index 4c4c5d1c748ef..e215dd59bd676 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryState.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryState.cs @@ -9,9 +9,11 @@ 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 [StructLayout(LayoutKind.Explicit)] - private struct State + private struct CacheEntryState { + // this field overlaps with the 4 other byte fields, it exists to allow for atomic updates [FieldOffset(0)] private int _state; @@ -24,7 +26,7 @@ private struct State [FieldOffset(3)] private byte _reserved; // for future use - internal State(CacheItemPriority priority) : this() => _priority = (byte)priority; + internal CacheEntryState(CacheItemPriority priority) : this() => _priority = (byte)priority; internal bool IsDisposed { get => ((Flags)_flags).HasFlag(Flags.IsDisposed); set => SetFlag(Flags.IsDisposed, value); } @@ -37,7 +39,7 @@ internal EvictionReason EvictionReason get => (EvictionReason)_evictionReason; set { - State before, after; + CacheEntryState before, after; do { before = this; @@ -52,7 +54,7 @@ internal CacheItemPriority Priority get => (CacheItemPriority)_priority; set { - State before, after; + CacheEntryState before, after; do { before = this; @@ -64,7 +66,7 @@ internal CacheItemPriority Priority private void SetFlag(Flags option, bool value) { - State before, after; + CacheEntryState before, after; do { diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryTokens.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryTokens.cs index a06d5ca052c14..1eab9689ac360 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryTokens.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryTokens.cs @@ -9,7 +9,9 @@ namespace Microsoft.Extensions.Caching.Memory { internal partial class CacheEntry { - private sealed class Tokens + // this type exists just to reduce average CacheEntry size + // which typicall is not using expiration tokens or callbacks + private sealed class CacheEntryTokens { internal List _expirationTokens; internal List _expirationTokenRegistrations; From c6597db7f63a615bd3325ae5f01f2f372c5cdc4c Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 11 Dec 2020 14:02:44 +0100 Subject: [PATCH 06/19] inline private properties that are used just twice in total --- .../src/CacheEntry.cs | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index 1b2b5cae259c8..52aaf334114b8 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -123,7 +123,7 @@ public object Value set { _value = value; - IsValueSet = true; + _state.IsValueSet = true; } } @@ -131,22 +131,16 @@ public object Value internal EvictionReason EvictionReason { get => _state.EvictionReason; private set => _state.EvictionReason = value; } - private bool IsDisposed { get => _state.IsDisposed; set => _state.IsDisposed = value; } - - private bool IsExpired { get => _state.IsExpired; set => _state.IsExpired = value; } - - private bool IsValueSet { get => _state.IsValueSet; set => _state.IsValueSet = 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,7 +157,7 @@ public void Dispose() [MethodImpl(MethodImplOptions.AggressiveInlining)] internal bool CheckExpired(in DateTimeOffset now) - => IsExpired + => _state.IsExpired || CheckForExpiredTime(now) || (_tokens != null && _tokens.CheckForExpiredTokens(this)); @@ -173,7 +167,7 @@ internal void SetExpired(EvictionReason reason) { EvictionReason = reason; } - IsExpired = true; + _state.IsExpired = true; _tokens?.DetachTokens(); } From c5196666880ff769336987d2745e260a1ebfc6a1 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 11 Dec 2020 15:01:17 +0100 Subject: [PATCH 07/19] force inline some methods based on profiling data --- .../Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs | 4 +++- .../src/CacheEntryState.cs | 8 +++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index 52aaf334114b8..3d4b3d27aff3c 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -155,7 +155,7 @@ public void Dispose() } } - [MethodImpl(MethodImplOptions.AggressiveInlining)] + [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling internal bool CheckExpired(in DateTimeOffset now) => _state.IsExpired || CheckForExpiredTime(now) @@ -171,6 +171,7 @@ internal void SetExpired(EvictionReason reason) _tokens?.DetachTokens(); } + [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling private bool CheckForExpiredTime(in DateTimeOffset now) { if (!AbsoluteExpiration.HasValue && !_slidingExpiration.HasValue) @@ -247,6 +248,7 @@ private static void InvokeCallbacks(CacheEntry entry) } // this simple check very often allows us to avoid expensive call to PropagateOptions(CacheEntryHelper.Current) + [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling internal bool CanPropagateOptions() => _tokens?._expirationTokens != null || AbsoluteExpiration.HasValue; internal void PropagateOptions(CacheEntry parent) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryState.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryState.cs index e215dd59bd676..dc36e891c4f94 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryState.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryState.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Threading; @@ -30,7 +31,12 @@ private struct CacheEntryState internal bool IsDisposed { get => ((Flags)_flags).HasFlag(Flags.IsDisposed); set => SetFlag(Flags.IsDisposed, value); } - internal bool IsExpired { get => ((Flags)_flags).HasFlag(Flags.IsExpired); set => SetFlag(Flags.IsExpired, value); } + internal bool IsExpired + { + [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling + get => ((Flags)_flags).HasFlag(Flags.IsExpired); + set => SetFlag(Flags.IsExpired, value); + } internal bool IsValueSet { get => ((Flags)_flags).HasFlag(Flags.IsValueSet); set => SetFlag(Flags.IsValueSet, value); } From 4a5715ac5f00275c8be80ff32003c447642fb86e Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 11 Dec 2020 16:47:53 +0100 Subject: [PATCH 08/19] rename files --- .../src/{CacheEntryState.cs => CacheEntry.CacheEntryState.cs} | 0 .../src/{CacheEntryTokens.cs => CacheEntry.CacheEntryTokens.cs} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename src/libraries/Microsoft.Extensions.Caching.Memory/src/{CacheEntryState.cs => CacheEntry.CacheEntryState.cs} (100%) rename src/libraries/Microsoft.Extensions.Caching.Memory/src/{CacheEntryTokens.cs => CacheEntry.CacheEntryTokens.cs} (100%) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryState.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs similarity index 100% rename from src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryState.cs rename to src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryTokens.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs similarity index 100% rename from src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryTokens.cs rename to src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs From f170e8dcfa740e7057cd145b1fbd3497670b3303 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 11 Dec 2020 16:56:34 +0100 Subject: [PATCH 09/19] apply code review suggestion --- .../src/CacheEntry.CacheEntryTokens.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs index 1eab9689ac360..a4f797f922732 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs @@ -10,7 +10,7 @@ namespace Microsoft.Extensions.Caching.Memory internal partial class CacheEntry { // this type exists just to reduce average CacheEntry size - // which typicall is not using expiration tokens or callbacks + // which typically is not using expiration tokens or callbacks private sealed class CacheEntryTokens { internal List _expirationTokens; From 01a268670b8ed79eeee8719ab4e9c73e583266bd Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 11 Dec 2020 18:21:28 +0100 Subject: [PATCH 10/19] apply code review suggestion: don't use Enum.HasFlag Co-authored-by: Jan Kotas --- .../src/CacheEntry.CacheEntryState.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs index dc36e891c4f94..56aabe3f8e2a3 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs @@ -33,8 +33,7 @@ private struct CacheEntryState internal bool IsExpired { - [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling - get => ((Flags)_flags).HasFlag(Flags.IsExpired); + get => ((Flags)_flags & Flags.IsExpired) != 0; set => SetFlag(Flags.IsExpired, value); } From 0c077312fc486256f4c5ee06e7696bb00facb680 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 11 Dec 2020 18:25:50 +0100 Subject: [PATCH 11/19] apply code review suggestion: don't use Enum.HasFlag --- .../src/CacheEntry.CacheEntryState.cs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs index 56aabe3f8e2a3..051e21a10797a 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs @@ -2,7 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; -using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Threading; @@ -29,7 +28,11 @@ private struct CacheEntryState internal CacheEntryState(CacheItemPriority priority) : this() => _priority = (byte)priority; - internal bool IsDisposed { get => ((Flags)_flags).HasFlag(Flags.IsDisposed); set => SetFlag(Flags.IsDisposed, value); } + internal bool IsDisposed + { + get => ((Flags)_flags & Flags.IsDisposed) != 0; + set => SetFlag(Flags.IsDisposed, value); + } internal bool IsExpired { @@ -37,7 +40,11 @@ internal bool IsExpired set => SetFlag(Flags.IsExpired, value); } - internal bool IsValueSet { get => ((Flags)_flags).HasFlag(Flags.IsValueSet); set => SetFlag(Flags.IsValueSet, value); } + internal bool IsValueSet + { + get => ((Flags)_flags & Flags.IsValueSet) != 0; + set => SetFlag(Flags.IsValueSet, value); + } internal EvictionReason EvictionReason { From 46ae2eadcaeed0e35166fe44302c1264413784bd Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 11 Dec 2020 18:33:07 +0100 Subject: [PATCH 12/19] apply code review suggestion: remove redundant Interlocked.CompareExchange --- .../src/CacheEntry.CacheEntryState.cs | 39 ++----------------- .../tests/CacheEntryScopeExpirationTests.cs | 30 ++++++++++++++ 2 files changed, 33 insertions(+), 36 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs index 051e21a10797a..ac91410521627 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs @@ -3,7 +3,6 @@ using System; using System.Runtime.InteropServices; -using System.Threading; namespace Microsoft.Extensions.Caching.Memory { @@ -13,10 +12,6 @@ internal partial class CacheEntry [StructLayout(LayoutKind.Explicit)] private struct CacheEntryState { - // this field overlaps with the 4 other byte fields, it exists to allow for atomic updates - [FieldOffset(0)] - private int _state; - [FieldOffset(0)] private byte _flags; [FieldOffset(1)] @@ -49,44 +44,16 @@ internal bool IsValueSet internal EvictionReason EvictionReason { get => (EvictionReason)_evictionReason; - set - { - CacheEntryState before, after; - do - { - before = this; - after = this; - after._evictionReason = (byte)value; - } while (Interlocked.CompareExchange(ref _state, after._state, before._state) != before._state); - } + set => _evictionReason = (byte)value; } internal CacheItemPriority Priority { get => (CacheItemPriority)_priority; - set - { - CacheEntryState before, after; - do - { - before = this; - after = this; - after._priority = (byte)value; - } while (Interlocked.CompareExchange(ref _state, after._state, before._state) != before._state); - } + set => _priority = (byte)value; } - private void SetFlag(Flags option, bool value) - { - CacheEntryState before, after; - - do - { - before = this; - after = this; - after._flags = (byte)(value ? (after._flags | (byte)option) : (after._flags & ~(byte)option)); - } while (Interlocked.CompareExchange(ref _state, after._state, before._state) != before._state); - } + private void SetFlag(Flags option, bool value) => _flags = (byte)(value ? (_flags | (byte)option) : (_flags & ~(byte)option)); [Flags] private enum Flags : byte diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs index 3ca46d0c024d6..9d34d63989f8d 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs @@ -482,5 +482,35 @@ await Task.WhenAll( 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)); + } + } + } } } From c3e75eac361ee87e4bc2265d7fb303d5933f0978 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 14 Dec 2020 08:56:29 +0100 Subject: [PATCH 13/19] address code review feedback: encapsulate the fields of CacheEntryTokens --- .../src/CacheEntry.CacheEntryTokens.cs | 45 +++++++++++++++++-- .../src/CacheEntry.cs | 37 +-------------- 2 files changed, 44 insertions(+), 38 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs index a4f797f922732..e19418ba9677f 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs @@ -3,6 +3,9 @@ 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 @@ -13,9 +16,9 @@ internal partial class CacheEntry // which typically is not using expiration tokens or callbacks private sealed class CacheEntryTokens { - internal List _expirationTokens; - internal List _expirationTokenRegistrations; - internal List _postEvictionCallbacks; // this is not really related to tokens, but was moved here to shrink typicall CacheEntry size + private List _expirationTokens; + private List _expirationTokenRegistrations; + private List _postEvictionCallbacks; // this is not really related to tokens, but was moved here to shrink typicall CacheEntry size internal List ExpirationTokens => _expirationTokens ??= new List(); internal List PostEvictionCallbacks => _postEvictionCallbacks ??= new List(); @@ -60,6 +63,8 @@ internal bool CheckForExpiredTokens(CacheEntry cacheEntry) return false; } + internal bool CanCopyTokens() => _expirationTokens != null; + internal void CopyTokens(CacheEntry parentEntry) { if (_expirationTokens != null) @@ -98,6 +103,40 @@ internal void DetachTokens() } } } + + 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) + { + IList 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"); + } + } + } } } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index 3d4b3d27aff3c..41fa179e0531d 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -6,7 +6,6 @@ using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; -using Microsoft.Extensions.Logging; using Microsoft.Extensions.Primitives; namespace Microsoft.Extensions.Caching.Memory @@ -213,43 +212,11 @@ private static void ExpirationTokensExpired(object obj) }, obj, CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default); } - internal void InvokeEvictionCallbacks() - { - if (_tokens?._postEvictionCallbacks != null) - { - Task.Factory.StartNew(state => InvokeCallbacks((CacheEntry)state), this, - CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default); - } - } - - private static void InvokeCallbacks(CacheEntry entry) - { - IList 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"); - } - } - } + internal void InvokeEvictionCallbacks() => _tokens?.InvokeEvictionCallbacks(this); // this simple check very often allows us to avoid expensive call to PropagateOptions(CacheEntryHelper.Current) [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling - internal bool CanPropagateOptions() => _tokens?._expirationTokens != null || AbsoluteExpiration.HasValue; + internal bool CanPropagateOptions() => (_tokens != null && _tokens.CanCopyTokens()) || AbsoluteExpiration.HasValue; internal void PropagateOptions(CacheEntry parent) { From f582eb6abf5725d27e7d01c97f7dab1ebf1f571b Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 5 Jan 2021 14:49:25 +0100 Subject: [PATCH 14/19] Apply suggestions from code review Co-authored-by: Eric Erhardt Co-authored-by: Stephen Toub --- .../src/CacheEntry.CacheEntryState.cs | 2 +- .../src/CacheEntry.CacheEntryTokens.cs | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs index ac91410521627..058b59e3f97cb 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs @@ -21,7 +21,7 @@ private struct CacheEntryState [FieldOffset(3)] private byte _reserved; // for future use - internal CacheEntryState(CacheItemPriority priority) : this() => _priority = (byte)priority; + internal CacheEntryState(CacheItemPriority priority) => _priority = (byte)priority; internal bool IsDisposed { diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs index e19418ba9677f..bf851eade5f9a 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs @@ -34,10 +34,7 @@ internal void AttachTokens() IChangeToken expirationToken = _expirationTokens[i]; if (expirationToken.ActiveChangeCallbacks) { - if (_expirationTokenRegistrations == null) - { - _expirationTokenRegistrations = new List(1); - } + _expirationTokenRegistrations ??= new List(1); IDisposable registration = expirationToken.RegisterChangeCallback(ExpirationCallback, this); _expirationTokenRegistrations.Add(registration); } From 6a39239058969a5cf9cdd3b6b77ecb78421b7e87 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 5 Jan 2021 14:53:21 +0100 Subject: [PATCH 15/19] there is no need for explicit memory layout anymore --- .../src/CacheEntry.CacheEntryState.cs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs index 058b59e3f97cb..51deed2933890 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs @@ -2,24 +2,17 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; -using System.Runtime.InteropServices; 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 - [StructLayout(LayoutKind.Explicit)] private struct CacheEntryState { - [FieldOffset(0)] private byte _flags; - [FieldOffset(1)] private byte _evictionReason; - [FieldOffset(2)] private byte _priority; - [FieldOffset(3)] - private byte _reserved; // for future use internal CacheEntryState(CacheItemPriority priority) => _priority = (byte)priority; From 58d6a0e23b81e7001d4c16536dd197c8542a0607 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 5 Jan 2021 14:54:36 +0100 Subject: [PATCH 16/19] there is no need for using IList abstraction (the field was changed from List to IList) --- .../src/CacheEntry.CacheEntryTokens.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs index bf851eade5f9a..7c34249074084 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs @@ -87,7 +87,7 @@ internal void DetachTokens() { lock (this) { - IList registrations = _expirationTokenRegistrations; + List registrations = _expirationTokenRegistrations; if (registrations != null) { _expirationTokenRegistrations = null; @@ -112,7 +112,7 @@ internal void InvokeEvictionCallbacks(CacheEntry cacheEntry) private static void InvokeCallbacks(CacheEntry entry) { - IList callbackRegistrations = Interlocked.Exchange(ref entry._tokens._postEvictionCallbacks, null); + List callbackRegistrations = Interlocked.Exchange(ref entry._tokens._postEvictionCallbacks, null); if (callbackRegistrations == null) { From a1fcbd1878ed202c6cc746b282a3a3897799a74d Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 5 Jan 2021 14:56:17 +0100 Subject: [PATCH 17/19] restore call to parameterless ctor to ensure that all struct fields are initialized --- .../src/CacheEntry.CacheEntryState.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs index 51deed2933890..77aac7b8a6d9e 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryState.cs @@ -14,7 +14,7 @@ private struct CacheEntryState private byte _evictionReason; private byte _priority; - internal CacheEntryState(CacheItemPriority priority) => _priority = (byte)priority; + internal CacheEntryState(CacheItemPriority priority) : this() => _priority = (byte)priority; internal bool IsDisposed { From 356e02fa4a340b4c0ad1b980a6073b75c163b70b Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 5 Jan 2021 14:59:32 +0100 Subject: [PATCH 18/19] CopyTokens => PropagateTokens for naming consistency with PropagateOptions --- .../src/CacheEntry.CacheEntryTokens.cs | 4 ++-- .../Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs index 7c34249074084..b279c76190cbb 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs @@ -60,9 +60,9 @@ internal bool CheckForExpiredTokens(CacheEntry cacheEntry) return false; } - internal bool CanCopyTokens() => _expirationTokens != null; + internal bool CanPropagateTokens() => _expirationTokens != null; - internal void CopyTokens(CacheEntry parentEntry) + internal void PropagateTokens(CacheEntry parentEntry) { if (_expirationTokens != null) { diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index 41fa179e0531d..606f0adac088e 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -216,7 +216,7 @@ private static void ExpirationTokensExpired(object obj) // this simple check very often allows us to avoid expensive call to PropagateOptions(CacheEntryHelper.Current) [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling - internal bool CanPropagateOptions() => (_tokens != null && _tokens.CanCopyTokens()) || AbsoluteExpiration.HasValue; + internal bool CanPropagateOptions() => (_tokens != null && _tokens.CanPropagateTokens()) || AbsoluteExpiration.HasValue; internal void PropagateOptions(CacheEntry parent) { @@ -227,7 +227,7 @@ internal void PropagateOptions(CacheEntry parent) // 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. - _tokens?.CopyTokens(parent); + _tokens?.PropagateTokens(parent); if (AbsoluteExpiration.HasValue) { From 9b189eb3f7d4a2432d22a7e8736277e6b8008993 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 7 Jan 2021 18:16:34 +0100 Subject: [PATCH 19/19] Apply suggestions from code review Co-authored-by: Eric Erhardt --- .../src/CacheEntry.CacheEntryTokens.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs index b279c76190cbb..5800a40118809 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs @@ -18,7 +18,7 @@ private sealed class CacheEntryTokens { private List _expirationTokens; private List _expirationTokenRegistrations; - private List _postEvictionCallbacks; // this is not really related to tokens, but was moved here to shrink typicall CacheEntry size + private List _postEvictionCallbacks; // this is not really related to tokens, but was moved here to shrink typical CacheEntry size internal List ExpirationTokens => _expirationTokens ??= new List(); internal List PostEvictionCallbacks => _postEvictionCallbacks ??= new List();