From 5c99d4d686a6dd7ad3f59df483a5e13290f3efd0 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Sat, 20 Apr 2024 11:13:43 +0100 Subject: [PATCH 1/8] HybridCache - post-PR fixes --- .../Hybrid/src/Internal/BufferChunk.cs | 7 +++++- .../src/Internal/DefaultHybridCache.Debug.cs | 20 ++++++++-------- .../DefaultHybridCache.MutableCacheItem.cs | 2 +- .../DefaultHybridCache.StampedeKey.cs | 8 +++++++ .../DefaultHybridCache.StampedeState.cs | 2 +- .../DefaultHybridCache.StampedeStateT.cs | 10 ++++---- .../Internal/RecyclableArrayBufferWriter.cs | 16 +++++++++++++ src/Caching/Hybrid/test/BufferReleaseTests.cs | 24 +++++++++---------- .../SqlServer/src/DatabaseOperations.cs | 2 +- 9 files changed, 60 insertions(+), 31 deletions(-) diff --git a/src/Caching/Hybrid/src/Internal/BufferChunk.cs b/src/Caching/Hybrid/src/Internal/BufferChunk.cs index c783810fcf30..73b89776c287 100644 --- a/src/Caching/Hybrid/src/Internal/BufferChunk.cs +++ b/src/Caching/Hybrid/src/Internal/BufferChunk.cs @@ -9,7 +9,8 @@ namespace Microsoft.Extensions.Caching.Hybrid.Internal; // used to convey buffer status; like ArraySegment, but Offset is always -// zero, and we use the MSB of the length to track whether or not to recycle this value +// zero, and we use the most significant bit (MSB, the sign flag) of the length +// to track whether or not to recycle this value internal readonly struct BufferChunk { private const int MSB = (1 << 31); @@ -40,6 +41,10 @@ public BufferChunk(byte[] array) Array = array; _lengthAndPoolFlag = array.Length; // assume not pooled, if exact-sized + // (we don't expect array.Length to be negative; we're really just saying + // "we expect the result of assigning array.Length to _lengthAndPoolFlag + // to give the expected Length *and* not have the MSB set; we're just + // checking that we haven't fat-fingered our MSB logic) Debug.Assert(!ReturnToPool, "do not return right-sized arrays"); Debug.Assert(Length == array.Length, "array length not respected"); } diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.Debug.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.Debug.cs index 0be6f768e5f4..9aaf3da4ed49 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.Debug.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.Debug.cs @@ -24,17 +24,17 @@ internal bool DebugTryGetCacheItem(string key, [NotNullWhen(true)] out CacheItem private int _outstandingBufferCount; - internal int DebugGetOutstandingBuffers(bool flush = false) + internal int DebugOnlyGetOutstandingBuffers(bool flush = false) => flush ? Interlocked.Exchange(ref _outstandingBufferCount, 0) : Volatile.Read(ref _outstandingBufferCount); [Conditional("DEBUG")] - internal void DebugDecrementOutstandingBuffers() + internal void DebugOnlyDecrementOutstandingBuffers() { Interlocked.Decrement(ref _outstandingBufferCount); } [Conditional("DEBUG")] - internal void DebugIncrementOutstandingBuffers() + internal void DebugOnlyIncrementOutstandingBuffers() { Interlocked.Increment(ref _outstandingBufferCount); } @@ -42,27 +42,27 @@ internal void DebugIncrementOutstandingBuffers() partial class MutableCacheItem { - partial void DebugDecrementOutstandingBuffers(); - partial void DebugTrackBufferCore(DefaultHybridCache cache); + partial void DebugOnlyDecrementOutstandingBuffers(); + partial void DebugOnlyTrackBufferCore(DefaultHybridCache cache); [Conditional("DEBUG")] - internal void DebugTrackBuffer(DefaultHybridCache cache) => DebugTrackBufferCore(cache); + internal void DebugOnlyTrackBuffer(DefaultHybridCache cache) => DebugOnlyTrackBufferCore(cache); #if DEBUG private DefaultHybridCache? _cache; // for buffer-tracking - only enabled in DEBUG - partial void DebugDecrementOutstandingBuffers() + partial void DebugOnlyDecrementOutstandingBuffers() { if (_buffer.ReturnToPool) { - _cache?.DebugDecrementOutstandingBuffers(); + _cache?.DebugOnlyDecrementOutstandingBuffers(); } } - partial void DebugTrackBufferCore(DefaultHybridCache cache) + partial void DebugOnlyTrackBufferCore(DefaultHybridCache cache) { _cache = cache; if (_buffer.ReturnToPool) { - _cache?.DebugIncrementOutstandingBuffers(); + _cache?.DebugOnlyIncrementOutstandingBuffers(); } } #endif diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.MutableCacheItem.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.MutableCacheItem.cs index ef99a5738091..5b1dd6b8cbaf 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.MutableCacheItem.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.MutableCacheItem.cs @@ -41,7 +41,7 @@ public override void Release() var newCount = Interlocked.Decrement(ref _refCount); if (newCount == 0) { - DebugDecrementOutstandingBuffers(); + DebugOnlyDecrementOutstandingBuffers(); _buffer.RecycleIfAppropriate(); } } diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeKey.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeKey.cs index bf5001360eb3..d22a6f08454b 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeKey.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeKey.cs @@ -15,6 +15,14 @@ partial class DefaultHybridCache private readonly int _hashCode; // we know we'll need it; compute it once only public StampedeKey(string key, HybridCacheEntryFlags flags) { + // We'll use both the key *and* the flags as combined flag; in reality, we *expect* + // the flags to be consistent between calls on the same operation, and it must be + // noted that the *cache items* only use the key (not the flags), but: it gets + // very hard to grok what the correct behaviour should be if combining two calls + // with different flags, since they could have mutually exclusive behaviours! + + // As such, we'll treat conflicting calls entirely separately from a stampede + // perspective. _key = key; _flags = flags; #if NETCOREAPP2_1_OR_GREATER || NETSTANDARD2_1_OR_GREATER diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeState.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeState.cs index f0a983093ac1..afeb1c9d011f 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeState.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeState.cs @@ -103,5 +103,5 @@ public bool TryAddCaller() // essentially just interlocked-increment, but with a } } - private void RemoveStampede(StampedeKey key) => _currentOperations.TryRemove(key, out _); + private void RemoveStampedeState(StampedeKey key) => _currentOperations.TryRemove(key, out _); } diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs index 7a76cdcdaf78..325cf9337601 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs @@ -134,7 +134,7 @@ private void SetException(Exception ex) { if (_result is not null) { - Cache.RemoveStampede(Key); + Cache.RemoveStampedeState(Key); _result.TrySetException(ex); } } @@ -148,7 +148,7 @@ private void SetResult(CacheItem value) if (_result is not null) { - Cache.RemoveStampede(Key); + Cache.RemoveStampedeState(Key); _result.TrySetResult(value); } } @@ -158,7 +158,7 @@ private void SetDefaultResult() // note we don't store this dummy result in L1 or L2 if (_result is not null) { - Cache.RemoveStampede(Key); + Cache.RemoveStampedeState(Key); _result.TrySetResult(ImmutableCacheItem.Default); } } @@ -180,7 +180,7 @@ private void SetResultAndRecycleIfAppropriate(ref BufferChunk value) { // use the buffer directly as the backing in the cache-item; do *not* recycle now var tmp = new MutableCacheItem(ref value, serializer); - tmp.DebugTrackBuffer(Cache); // conditional: DEBUG + tmp.DebugOnlyTrackBuffer(Cache); cacheItem = tmp; } @@ -198,7 +198,7 @@ private CacheItem SetResult(T value) else { var tmp = new MutableCacheItem(value, Cache.GetSerializer(), MaximumPayloadBytes); // serialization happens here - tmp.DebugTrackBuffer(Cache); // conditional: DEBUG + tmp.DebugOnlyTrackBuffer(Cache); cacheItem = tmp; } SetResult(cacheItem); diff --git a/src/Caching/Hybrid/src/Internal/RecyclableArrayBufferWriter.cs b/src/Caching/Hybrid/src/Internal/RecyclableArrayBufferWriter.cs index 5ff6d4699db8..9c65a33863a5 100644 --- a/src/Caching/Hybrid/src/Internal/RecyclableArrayBufferWriter.cs +++ b/src/Caching/Hybrid/src/Internal/RecyclableArrayBufferWriter.cs @@ -13,6 +13,22 @@ namespace Microsoft.Extensions.Caching.Hybrid.Internal; // except it uses the array pool for allocations internal sealed class RecyclableArrayBufferWriter : IBufferWriter, IDisposable { + // Usage note: *normally* you might want to use "using" for this, and that is fine; + // however, caution should be exercised in exception scenarios where we don't 100% + // know that the caller has stopped touching the buffer; in particular, this means + // scenarios involving a combination of external code and (for example) "async". + // In those cases, it may be preferable to manually dispose in the success case, + // and just drop the buffers in the failure case, i.e. instead of: + // + // using (writer) + // { DoStuff(); } + // + // simply: + // + // DoStuff(); + // writer.Dispose(); + // + // This does not represent a problem, and is consistent with many ArrayPool use-cases. // Copy of Array.MaxLength. // Used by projects targeting .NET Framework. diff --git a/src/Caching/Hybrid/test/BufferReleaseTests.cs b/src/Caching/Hybrid/test/BufferReleaseTests.cs index f83ce760c3df..58cd7f3c8fad 100644 --- a/src/Caching/Hybrid/test/BufferReleaseTests.cs +++ b/src/Caching/Hybrid/test/BufferReleaseTests.cs @@ -29,17 +29,17 @@ public async Task BufferGetsReleased_NoL2() { using var provider = GetDefaultCache(out var cache); #if DEBUG - cache.DebugGetOutstandingBuffers(flush: true); + cache.DebugOnlyGetOutstandingBuffers(flush: true); #endif var key = Me(); #if DEBUG - Assert.Equal(0, cache.DebugGetOutstandingBuffers()); + Assert.Equal(0, cache.DebugOnlyGetOutstandingBuffers()); #endif var first = await cache.GetOrCreateAsync(key, _ => GetAsync()); Assert.NotNull(first); #if DEBUG - Assert.Equal(1, cache.DebugGetOutstandingBuffers()); + Assert.Equal(1, cache.DebugOnlyGetOutstandingBuffers()); #endif Assert.True(cache.DebugTryGetCacheItem(key, out var cacheItem)); @@ -62,7 +62,7 @@ public async Task BufferGetsReleased_NoL2() await Task.Delay(250); } #if DEBUG - Assert.Equal(0, cache.DebugGetOutstandingBuffers()); + Assert.Equal(0, cache.DebugOnlyGetOutstandingBuffers()); #endif // assert that we can *no longer* reserve this buffer, because we've already recycled it Assert.False(cacheItem.TryReserveBuffer(out _)); @@ -116,13 +116,13 @@ static bool Write(IBufferWriter destination, byte[]? buffer) cache.BackendCache.Set(key, writer.ToArray()); } #if DEBUG - cache.DebugGetOutstandingBuffers(flush: true); - Assert.Equal(0, cache.DebugGetOutstandingBuffers()); + cache.DebugOnlyGetOutstandingBuffers(flush: true); + Assert.Equal(0, cache.DebugOnlyGetOutstandingBuffers()); #endif var first = await cache.GetOrCreateAsync(key, _ => GetAsync(), _noUnderlying); // we expect this to come from L2, hence NoUnderlying Assert.NotNull(first); #if DEBUG - Assert.Equal(0, cache.DebugGetOutstandingBuffers()); + Assert.Equal(0, cache.DebugOnlyGetOutstandingBuffers()); #endif Assert.True(cache.DebugTryGetCacheItem(key, out var cacheItem)); @@ -146,7 +146,7 @@ static bool Write(IBufferWriter destination, byte[]? buffer) await Task.Delay(250); } #if DEBUG - Assert.Equal(0, cache.DebugGetOutstandingBuffers()); + Assert.Equal(0, cache.DebugOnlyGetOutstandingBuffers()); #endif // assert that we can *no longer* reserve this buffer, because we've already recycled it Assert.True(cacheItem.TryReserveBuffer(out _)); // always readable @@ -172,13 +172,13 @@ static bool Write(IBufferWriter destination, byte[]? buffer) cache.BackendCache.Set(key, writer.ToArray()); } #if DEBUG - cache.DebugGetOutstandingBuffers(flush: true); - Assert.Equal(0, cache.DebugGetOutstandingBuffers()); + cache.DebugOnlyGetOutstandingBuffers(flush: true); + Assert.Equal(0, cache.DebugOnlyGetOutstandingBuffers()); #endif var first = await cache.GetOrCreateAsync(key, _ => GetAsync(), _noUnderlying); // we expect this to come from L2, hence NoUnderlying Assert.NotNull(first); #if DEBUG - Assert.Equal(1, cache.DebugGetOutstandingBuffers()); + Assert.Equal(1, cache.DebugOnlyGetOutstandingBuffers()); #endif Assert.True(cache.DebugTryGetCacheItem(key, out var cacheItem)); @@ -202,7 +202,7 @@ static bool Write(IBufferWriter destination, byte[]? buffer) await Task.Delay(250); } #if DEBUG - Assert.Equal(0, cache.DebugGetOutstandingBuffers()); + Assert.Equal(0, cache.DebugOnlyGetOutstandingBuffers()); #endif // assert that we can *no longer* reserve this buffer, because we've already recycled it Assert.False(cacheItem.TryReserveBuffer(out _)); // released now diff --git a/src/Caching/SqlServer/src/DatabaseOperations.cs b/src/Caching/SqlServer/src/DatabaseOperations.cs index 0adbd9d54128..cd6f7f2aa404 100644 --- a/src/Caching/SqlServer/src/DatabaseOperations.cs +++ b/src/Caching/SqlServer/src/DatabaseOperations.cs @@ -308,7 +308,7 @@ public void SetCacheItem(string key, ArraySegment value, DistributedCacheE return value; } - static long StreamOut(SqlDataReader source, int ordinal, IBufferWriter destination) + private static long StreamOut(SqlDataReader source, int ordinal, IBufferWriter destination) { long dataIndex = 0; int read = 0; From eeaf8571b17348aebd51be46597642e2d2b5bb88 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Mon, 22 Apr 2024 09:44:53 +0100 Subject: [PATCH 2/8] use partitioned lock to avoid minor race condition on "add" vs "remove" paths --- .../Internal/DefaultHybridCache.Stampede.cs | 2 +- .../DefaultHybridCache.StampedeKey.cs | 6 +++- .../DefaultHybridCache.StampedeState.cs | 17 +++++++--- .../DefaultHybridCache.StampedeStateT.cs | 6 ++-- .../Internal/DefaultHybridCache.SyncLock.cs | 34 +++++++++++++++++++ src/Caching/Hybrid/test/StampedeTests.cs | 32 +++++++++++++++++ 6 files changed, 88 insertions(+), 9 deletions(-) create mode 100644 src/Caching/Hybrid/src/Internal/DefaultHybridCache.SyncLock.cs diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.Stampede.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.Stampede.cs index 01d797a08194..bb05952fd29a 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.Stampede.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.Stampede.cs @@ -43,7 +43,7 @@ public bool GetOrCreateStampedeState(string key, HybridCacheEntryFlag // hmm; failed to add - there's concurrent activity on the same key; we're now // in very rare race condition territory; go ahead and take a lock while we // collect our thoughts - lock (_currentOperations) + lock (GetPartitionedSyncLock(in stampedeKey)) // see notes in SyncLock.cs { // check again while we hold the lock if (TryJoinExistingSession(this, stampedeKey, out existing)) diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeKey.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeKey.cs index d22a6f08454b..eeca9e589b13 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeKey.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeKey.cs @@ -26,7 +26,7 @@ public StampedeKey(string key, HybridCacheEntryFlags flags) _key = key; _flags = flags; #if NETCOREAPP2_1_OR_GREATER || NETSTANDARD2_1_OR_GREATER - _hashCode = HashCode.Combine(key, flags); + _hashCode = System.HashCode.Combine(key, flags); #else _hashCode = key.GetHashCode() ^ (int)flags; #endif @@ -35,6 +35,10 @@ public StampedeKey(string key, HybridCacheEntryFlags flags) public string Key => _key; public HybridCacheEntryFlags Flags => _flags; + // allow direct access to the pre-computed hash-code, semantically emphasizing that + // this is a constant-time operation against a known value + internal int HashCode => _hashCode; + public bool Equals(StampedeKey other) => _flags == other._flags & _key == other._key; public override bool Equals([NotNullWhen(true)] object? obj) diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeState.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeState.cs index afeb1c9d011f..6a23ea209e64 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeState.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeState.cs @@ -22,7 +22,10 @@ internal abstract class StampedeState private readonly CancellationTokenSource? _sharedCancellation; internal readonly CancellationToken SharedToken; // this might have a value even when _sharedCancellation is null - public StampedeKey Key { get; } + // we expose the key as a by-ref readonly; this minimizes the stack work involved in passing the key around + // (both in terms of width and copy-semantics) + private readonly StampedeKey _key; + public ref readonly StampedeKey Key => ref _key; /// /// Create a stamped token optionally with shared cancellation support @@ -30,7 +33,7 @@ internal abstract class StampedeState protected StampedeState(DefaultHybridCache cache, in StampedeKey key, bool canBeCanceled) { _cache = cache; - Key = key; + _key = key; if (canBeCanceled) { // if the first (or any) caller can't be cancelled; we'll never get to zero; no point tracking @@ -50,7 +53,7 @@ protected StampedeState(DefaultHybridCache cache, in StampedeKey key, bool canBe protected StampedeState(DefaultHybridCache cache, in StampedeKey key, CancellationToken token) { _cache = cache; - Key = key; + _key = key; SharedToken = token; } @@ -103,5 +106,11 @@ public bool TryAddCaller() // essentially just interlocked-increment, but with a } } - private void RemoveStampedeState(StampedeKey key) => _currentOperations.TryRemove(key, out _); + private void RemoveStampedeState(in StampedeKey key) + { + lock (GetPartitionedSyncLock(in key)) // see notes in SyncLock.cs + { + _currentOperations.TryRemove(key, out _); + } + } } diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs index 325cf9337601..ce5ba55336f1 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs @@ -134,7 +134,7 @@ private void SetException(Exception ex) { if (_result is not null) { - Cache.RemoveStampedeState(Key); + Cache.RemoveStampedeState(in Key); _result.TrySetException(ex); } } @@ -148,7 +148,7 @@ private void SetResult(CacheItem value) if (_result is not null) { - Cache.RemoveStampedeState(Key); + Cache.RemoveStampedeState(in Key); _result.TrySetResult(value); } } @@ -158,7 +158,7 @@ private void SetDefaultResult() // note we don't store this dummy result in L1 or L2 if (_result is not null) { - Cache.RemoveStampedeState(Key); + Cache.RemoveStampedeState(in Key); _result.TrySetResult(ImmutableCacheItem.Default); } } diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.SyncLock.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.SyncLock.cs new file mode 100644 index 000000000000..1837a6e7e2f3 --- /dev/null +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.SyncLock.cs @@ -0,0 +1,34 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace Microsoft.Extensions.Caching.Hybrid.Internal; + +partial class DefaultHybridCache +{ + // HybridCache's stampede protection requires some level of synchronization to avoid unnecessary runs + // of the underlying data fetch; this is *minimized* by the use of double-checked locking and + // interlocked join (adding a new request to an existing execution), but: that would leave a race + // condition where the *remove* step of the stampede would be in a race with the *add new* step; the + // *add new* step is inside a lock, but we need to *remove* step to share that lock, to avoid + // the race. We deal with that by taking the same lock during remove, but *that* means we're locking + // on all executions. + // + // To minimize lock contention, we will therefore use partitioning of the lock-token, by using the + // low 3 bits of the hash-code (which we calculate eagerly only one, so: already know). This gives + // us a fast way to split contention 8, almost an order-of-magnitude, which is sufficient. We *could* + // use an array for this, but: for directness, let's inline it instead (avoiding bounds-checks, + // an extra layer of dereferencing, and the allocation; I will acknowledge these are miniscule, but: + // it costs us nothing to do) + + private readonly object _syncLock0 = new(), _syncLock1 = new(), _syncLock2 = new(), _syncLock3 = new(), + _syncLock4 = new(), _syncLock5 = new(), _syncLock6 = new(), _syncLock7 = new(); + + internal object GetPartitionedSyncLock(in StampedeKey key) + => (key.HashCode & 0b111) switch // generate 8 partitions using the low 3 bits + { + 0 => _syncLock0, 1 => _syncLock1, + 2 => _syncLock2, 3 => _syncLock3, + 4 => _syncLock4, 5 => _syncLock5, + 6 => _syncLock6, _ => _syncLock7, + }; +} diff --git a/src/Caching/Hybrid/test/StampedeTests.cs b/src/Caching/Hybrid/test/StampedeTests.cs index b6536655a5e3..833ddd7e4a67 100644 --- a/src/Caching/Hybrid/test/StampedeTests.cs +++ b/src/Caching/Hybrid/test/StampedeTests.cs @@ -365,6 +365,38 @@ public async Task MutableTypesNeverShareFinalTask(bool withCancelation) Assert.NotSame(x, y); } + [Fact] + public void ValidatePartitioning() + { + // we just want to validate that key-level partitioning is + // happening to some degree, i.e. it isn't fundamentally broken + using var scope = GetDefaultCache(out var cache); + Dictionary counts = []; + for(int i = 0; i < 1024; i++) + { + var key = new DefaultHybridCache.StampedeKey(Guid.NewGuid().ToString(), default); + var obj = cache.GetPartitionedSyncLock(in key); + if (!counts.TryGetValue(obj, out var count)) + { + count = 0; + } + counts[obj] = count + 1; + } + + // We just want to prove that we got 8 non-empty partitions. + // This is *technically* non-deterministic, but: we'd + // need to be having a very bad day for the math gods + // to conspire against us that badly - if this test + // starts failing, maybe buy a lottery ticket? + Assert.Equal(8, counts.Count); + foreach (var pair in counts) + { + // the *median* should be 128 here; let's + // not be aggressive about it, though + Assert.True(pair.Value > 16); + } + } + class Mutable(Guid value) { public Guid Value => value; From 063dde4ec0cc690a9a406699546ec40776da0d36 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Mon, 22 Apr 2024 09:52:12 +0100 Subject: [PATCH 3/8] tyop --- .../Hybrid/src/Internal/DefaultHybridCache.SyncLock.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.SyncLock.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.SyncLock.cs index 1837a6e7e2f3..c702f7e9b3f9 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.SyncLock.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.SyncLock.cs @@ -14,8 +14,8 @@ partial class DefaultHybridCache // on all executions. // // To minimize lock contention, we will therefore use partitioning of the lock-token, by using the - // low 3 bits of the hash-code (which we calculate eagerly only one, so: already know). This gives - // us a fast way to split contention 8, almost an order-of-magnitude, which is sufficient. We *could* + // low 3 bits of the hash-code (which we calculate eagerly only once, so: already known). This gives + // us a fast way to split contention by 8, almost an order-of-magnitude, which is sufficient. We *could* // use an array for this, but: for directness, let's inline it instead (avoiding bounds-checks, // an extra layer of dereferencing, and the allocation; I will acknowledge these are miniscule, but: // it costs us nothing to do) From 7f8fbc641633693678ded1f2feab355bac7bda8e Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Mon, 22 Apr 2024 10:39:08 +0100 Subject: [PATCH 4/8] 1. double-check any use L1 inside the partitioned lock, to allow for the fact that an outgoing previous operation may have added it 2. remove any reference to IDisposable; that creates semantically invalid outcomes --- .../Internal/DefaultHybridCache.CacheItem.cs | 5 +- .../DefaultHybridCache.ImmutableCacheItem.cs | 9 - .../DefaultHybridCache.Serialization.cs | 2 - .../Internal/DefaultHybridCache.Stampede.cs | 17 ++ .../DefaultHybridCache.StampedeStateT.cs | 4 + .../Hybrid/test/DisposableValueTests.cs | 188 ------------------ 6 files changed, 23 insertions(+), 202 deletions(-) delete mode 100644 src/Caching/Hybrid/test/DisposableValueTests.cs diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.CacheItem.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.CacheItem.cs index 7dba0f76d6be..fb3b22507f3e 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.CacheItem.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.CacheItem.cs @@ -14,15 +14,14 @@ internal abstract class CacheItem { if (value is CacheItem item) { - // perform per-item clean-up; this could be buffer recycling (if defensive copies needed), - // or could be disposal + // perform per-item clean-up, i.e. buffer recycling (if defensive copies needed) item.OnEviction(); } }; public virtual void Release() { } // for recycling purposes - public abstract bool NeedsEvictionCallback { get; } // do we need to call Release when evicted? + public virtual bool NeedsEvictionCallback => false; // do we need to call Release when evicted? public virtual void OnEviction() { } // only invoked if NeedsEvictionCallback reported true diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.ImmutableCacheItem.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.ImmutableCacheItem.cs index f87335674b95..b9138893b84e 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.ImmutableCacheItem.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.ImmutableCacheItem.cs @@ -19,15 +19,6 @@ partial class DefaultHybridCache // this is only used when the underlying store is disabled; we don't need 100% singleton; "good enough is" public static ImmutableCacheItem Default => _sharedDefault ??= new(default!); - public override void OnEviction() - { - var obj = _value as IDisposable; - Debug.Assert(obj is not null, "shouldn't be here for non-disposable types"); - obj?.Dispose(); - } - - public override bool NeedsEvictionCallback => ImmutableTypeCache.IsDisposable; - public override bool TryGetValue(out T value) { value = _value; diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.Serialization.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.Serialization.cs index 42b5789e4b86..0adf22b793be 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.Serialization.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.Serialization.cs @@ -57,8 +57,6 @@ internal static class ImmutableTypeCache // lazy memoize; T doesn't change pe { // note for blittable types: a pure struct will be a full copy every time - nothing shared to mutate public static readonly bool IsImmutable = (typeof(T).IsValueType && IsBlittable()) || IsImmutable(typeof(T)); - - public static bool IsDisposable => typeof(IDisposable).IsAssignableFrom(typeof(T)); } private static bool IsBlittable() // minimize the generic portion diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.Stampede.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.Stampede.cs index bb05952fd29a..e975d8a2d922 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.Stampede.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.Stampede.cs @@ -57,6 +57,23 @@ public bool GetOrCreateStampedeState(string key, HybridCacheEntryFlag // the floor; in the grand scheme of things, that's OK; this is a rare outcome } + // and check whether the value was L1-cached by an outgoing operation (for *us* to check needs local-cache-read, + // and for *them* to have updated needs local-cache-write, but since the shared us/them key includes flags, + // we can skip this if *either* flag is set) + if ((flags & HybridCacheEntryFlags.DisableLocalCache) == 0 && _localCache.TryGetValue(key, out var untyped) + && untyped is CacheItem typed && typed.TryGetValue(out var value)) + { + // set the value against the *current* stampede-state, essentially making it pre-completed; we + // can emulate that by using ImmutableCacheItem (if it isn't already), noting that this state + // will never be in the dictionary, so this value is never shared with anyone else + if (typed is not ImmutableCacheItem immutable) + { + immutable = new ImmutableCacheItem(value); + } + stampedeState.SetResultDirect(immutable); + return false; // the work has ALREADY been done + } + // otherwise, either nothing existed - or the thing that already exists can't be joined; // in that case, go ahead and use the state that we invented a moment ago (outside of the lock) _currentOperations[stampedeKey] = stampedeState; diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs index ce5ba55336f1..2a58703aaf3a 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs @@ -139,6 +139,10 @@ private void SetException(Exception ex) } } + // ONLY set the result, without any other side-effects + internal void SetResultDirect(CacheItem value) + => _result?.TrySetResult(value); + private void SetResult(CacheItem value) { if ((Key.Flags & HybridCacheEntryFlags.DisableLocalCacheWrite) == 0) diff --git a/src/Caching/Hybrid/test/DisposableValueTests.cs b/src/Caching/Hybrid/test/DisposableValueTests.cs deleted file mode 100644 index 57b2e950485b..000000000000 --- a/src/Caching/Hybrid/test/DisposableValueTests.cs +++ /dev/null @@ -1,188 +0,0 @@ -// 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.ComponentModel; -using System.Linq; -using System.Runtime.CompilerServices; -using System.Text; -using System.Threading.Tasks; -using Microsoft.Extensions.Caching.Hybrid.Internal; -using Microsoft.Extensions.DependencyInjection; - -namespace Microsoft.Extensions.Caching.Hybrid.Tests; -public class DisposableValueTests -{ - // We can only reasonable be expected to be responsible for disposal (IDisposable) of objects - // if we're keeping hold of references, which means: things we consider immutable. - // It is noted that this creates an oddity whereby for *mutable* types, the caller needs to dispose - // per fetch (GetOrCreateAsync), and for *immutable* types: they're not - but that is unavoidable. - // In reality, it is expected to be pretty rare to hold disposable types here. - - private static ServiceProvider GetCache(out DefaultHybridCache cache) - { - var services = new ServiceCollection(); - services.AddHybridCache(); - var provider = services.BuildServiceProvider(); - cache = Assert.IsType(provider.GetRequiredService()); - return provider; - } - - [Fact] - public async void NonDisposableImmutableTypeDoesNotNeedEvictionCallback() - { - using var provider = GetCache(out var cache); - var key = Me(); - - var s = await cache.GetOrCreateAsync(key, _ => GetSomeString()); - Assert.NotNull(s); - Assert.False(string.IsNullOrWhiteSpace(s)); - Assert.True(cache.DebugTryGetCacheItem(key, out var cacheItem)); - Assert.True(cacheItem.DebugIsImmutable); - Assert.False(cacheItem.NeedsEvictionCallback); - - static ValueTask GetSomeString() => new(Guid.NewGuid().ToString()); - } - - [Fact] - public async void NonDisposableBlittableTypeDoesNotNeedEvictionCallback() - { - using var provider = GetCache(out var cache); - var key = Me(); - - var g = await cache.GetOrCreateAsync(key, _ => GetSomeGuid()); - Assert.NotEqual(Guid.Empty, g); - Assert.True(cache.DebugTryGetCacheItem(key, out var cacheItem)); - Assert.True(cacheItem.DebugIsImmutable); - Assert.False(cacheItem.NeedsEvictionCallback); - - static ValueTask GetSomeGuid() => new(Guid.NewGuid()); - } - - [Fact] - public async Task DispsableRefTypeNeedsEvictionCallback() - { - using var provider = GetCache(out var cache); - var key = Me(); - - var inst = new DisposableTestClass(); - var obj = await cache.GetOrCreateAsync(key, _ => new ValueTask(inst)); - Assert.Same(inst, obj); - Assert.True(cache.DebugTryGetCacheItem(key, out var cacheItem)); - Assert.True(cacheItem.DebugIsImmutable); - Assert.True(cacheItem.NeedsEvictionCallback); - - Assert.Equal(0, inst.DisposeCount); - - // now remove it - await cache.RemoveKeyAsync(key); - - // give it a moment for the eviction callback to kick in - for (var i = 0; i < 10; i++) - { - await Task.Delay(250); - if (inst.DisposeCount != 0) - { - break; - } - } - Assert.Equal(1, inst.DisposeCount); - } - - [Fact] - public async Task DisposableValueTypeNeedsEvictionCallback() - { - using var provider = GetCache(out var cache); - var key = Me(); - - // disposal of value-type - var inst = new DisposableTestClass(); - var v = await cache.GetOrCreateAsync(key, _ => new ValueTask(new DisposableTestValue(inst))); - Assert.Same(inst, v.Wrapped); - Assert.True(cache.DebugTryGetCacheItem(key, out var cacheItem)); - Assert.True(cacheItem.DebugIsImmutable); - Assert.True(cacheItem.NeedsEvictionCallback); - - Assert.Equal(0, inst.DisposeCount); - - // now remove it - await cache.RemoveKeyAsync(key); - - // give it a moment for the eviction callback to kick in - for (var i = 0; i < 10; i++) - { - await Task.Delay(250); - if (inst.DisposeCount != 0) - { - break; - } - } - Assert.Equal(1, inst.DisposeCount); - } - - [Fact] - public async Task NonDispsableRefTypeDoesNotNeedEvictionCallback() - { - using var provider = GetCache(out var cache); - var key = Me(); - - var inst = new NonDisposableTestClass(); - var obj = await cache.GetOrCreateAsync(key, _ => new ValueTask(inst)); - Assert.Same(inst, obj); - Assert.True(cache.DebugTryGetCacheItem(key, out var cacheItem)); - Assert.True(cacheItem.DebugIsImmutable); - Assert.False(cacheItem.NeedsEvictionCallback); - } - - [Fact] - public async Task NonDisposableValueTypeDoesNotNeedEvictionCallback() - { - using var provider = GetCache(out var cache); - var key = Me(); - - // disposal of value-type - var inst = new DisposableTestClass(); - var v = await cache.GetOrCreateAsync(key, _ => new ValueTask(new NonDisposableTestValue(inst))); - Assert.Same(inst, v.Wrapped); - Assert.True(cache.DebugTryGetCacheItem(key, out var cacheItem)); - Assert.True(cacheItem.DebugIsImmutable); - Assert.False(cacheItem.NeedsEvictionCallback); - } - - [ImmutableObject(true)] - public sealed class DisposableTestClass : IDisposable - { - private int _disposeCount; - public void Dispose() => Interlocked.Increment(ref _disposeCount); - - public int DisposeCount => Volatile.Read(ref _disposeCount); - } - - [ImmutableObject(true)] - public readonly struct DisposableTestValue : IDisposable - { - public DisposableTestClass Wrapped { get; } - public DisposableTestValue(DisposableTestClass inner) => Wrapped = inner; - public void Dispose() => Wrapped.Dispose(); - } - - [ImmutableObject(true)] - public sealed class NonDisposableTestClass - { - private int _disposeCount; - public void Dispose() => Interlocked.Increment(ref _disposeCount); - - public int DisposeCount => Volatile.Read(ref _disposeCount); - } - - [ImmutableObject(true)] - public readonly struct NonDisposableTestValue - { - public DisposableTestClass Wrapped { get; } - public NonDisposableTestValue(DisposableTestClass inner) => Wrapped = inner; - public void Dispose() => Wrapped.Dispose(); - } - - private static string Me([CallerMemberName] string caller = "") => caller; -} From 223f1bd8ba158328b0d9c9af35035825fd159b0c Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Mon, 22 Apr 2024 12:31:03 +0100 Subject: [PATCH 5/8] avoid deserialize inside lock, involves refactoring cache item so we can reserve against a cache-item --- .../src/HybridCacheBuilderExtensions.cs | 5 -- src/Caching/Hybrid/src/HybridCacheOptions.cs | 7 -- .../src/HybridCacheServiceExtensions.cs | 6 -- src/Caching/Hybrid/src/IHybridCacheBuilder.cs | 5 -- .../Internal/DefaultHybridCache.CacheItem.cs | 56 ++++++++++++-- .../DefaultHybridCache.ImmutableCacheItem.cs | 7 +- .../src/Internal/DefaultHybridCache.L2.cs | 15 ++-- .../DefaultHybridCache.MutableCacheItem.cs | 44 ++--------- .../Internal/DefaultHybridCache.Stampede.cs | 11 +-- .../DefaultHybridCache.StampedeState.cs | 32 +++----- .../DefaultHybridCache.StampedeStateT.cs | 75 ++++++++++++------- .../Hybrid/src/Internal/DefaultHybridCache.cs | 2 +- src/Caching/Hybrid/test/BufferReleaseTests.cs | 6 ++ 13 files changed, 134 insertions(+), 137 deletions(-) diff --git a/src/Caching/Hybrid/src/HybridCacheBuilderExtensions.cs b/src/Caching/Hybrid/src/HybridCacheBuilderExtensions.cs index a27240f66418..4c7dc27c72e2 100644 --- a/src/Caching/Hybrid/src/HybridCacheBuilderExtensions.cs +++ b/src/Caching/Hybrid/src/HybridCacheBuilderExtensions.cs @@ -1,12 +1,7 @@ // 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.Diagnostics.CodeAnalysis; -using System.Linq; -using System.Text; -using System.Threading.Tasks; using Microsoft.Extensions.DependencyInjection; namespace Microsoft.Extensions.Caching.Hybrid; diff --git a/src/Caching/Hybrid/src/HybridCacheOptions.cs b/src/Caching/Hybrid/src/HybridCacheOptions.cs index 66793746a5dd..65c0709764fc 100644 --- a/src/Caching/Hybrid/src/HybridCacheOptions.cs +++ b/src/Caching/Hybrid/src/HybridCacheOptions.cs @@ -1,13 +1,6 @@ // 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.Linq; -using System.Text; -using System.Threading.Tasks; -using Microsoft.Extensions.Options; - namespace Microsoft.Extensions.Caching.Hybrid; /// diff --git a/src/Caching/Hybrid/src/HybridCacheServiceExtensions.cs b/src/Caching/Hybrid/src/HybridCacheServiceExtensions.cs index fcb7e4ae4d57..87bdfd1cda1a 100644 --- a/src/Caching/Hybrid/src/HybridCacheServiceExtensions.cs +++ b/src/Caching/Hybrid/src/HybridCacheServiceExtensions.cs @@ -2,15 +2,9 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; -using System.Collections.Generic; -using System.Diagnostics.CodeAnalysis; -using System.Linq; -using System.Text; -using System.Threading.Tasks; using Microsoft.Extensions.Caching.Hybrid.Internal; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection.Extensions; -using Microsoft.Extensions.Internal; namespace Microsoft.Extensions.Caching.Hybrid; diff --git a/src/Caching/Hybrid/src/IHybridCacheBuilder.cs b/src/Caching/Hybrid/src/IHybridCacheBuilder.cs index fae49c030fc3..c46c8b650858 100644 --- a/src/Caching/Hybrid/src/IHybridCacheBuilder.cs +++ b/src/Caching/Hybrid/src/IHybridCacheBuilder.cs @@ -1,11 +1,6 @@ // 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.Linq; -using System.Text; -using System.Threading.Tasks; using Microsoft.Extensions.DependencyInjection; namespace Microsoft.Extensions.Caching.Hybrid; diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.CacheItem.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.CacheItem.cs index fb3b22507f3e..3893836decc4 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.CacheItem.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.CacheItem.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Threading; using Microsoft.Extensions.Caching.Memory; namespace Microsoft.Extensions.Caching.Hybrid.Internal; @@ -10,36 +11,79 @@ partial class DefaultHybridCache { internal abstract class CacheItem { + private int _refCount = 1; // the number of pending operations against this cache item + // note: the ref count is the number of callers anticipating this value at any given time; initially, + // it is one for a simple "get the value" flow, but if another call joins with us, it'll be incremented; + // if either cancels, it will get decremented, with the entire flow being cancelled if it ever becomes + // zero + // this counter also drives cache lifetime, with the cache itself incrementing the count by one; in the + // case of mutable data, cache eviction may reduce this to zero (in cooperation with any concurrent readers, + // who incr/decr around their fetch), allowing safe buffer recycling + + internal int RefCount => Volatile.Read(ref _refCount); + internal static readonly PostEvictionDelegate _sharedOnEviction = static (key, value, reason, state) => { if (value is CacheItem item) { - // perform per-item clean-up, i.e. buffer recycling (if defensive copies needed) - item.OnEviction(); + item.Release(); } }; - public virtual void Release() { } // for recycling purposes - public virtual bool NeedsEvictionCallback => false; // do we need to call Release when evicted? - public virtual void OnEviction() { } // only invoked if NeedsEvictionCallback reported true + protected virtual void OnFinalRelease() { } // any required release semantics public abstract bool TryReserveBuffer(out BufferChunk buffer); public abstract bool DebugIsImmutable { get; } + + public bool Release() // returns true ONLY for the final release step + { + var newCount = Interlocked.Decrement(ref _refCount); + if (newCount == 0) + { + // perform per-item clean-up, i.e. buffer recycling (if defensive copies needed) + OnFinalRelease(); + return true; + } + return false; + } + + public bool TryReserve() + { + var oldValue = Volatile.Read(ref _refCount); + do + { + if (oldValue is 0 or -1) + { + return false; // already burned, or about to roll around back to zero + } + + var updated = Interlocked.CompareExchange(ref _refCount, oldValue + 1, oldValue); + if (updated == oldValue) + { + return true; // we exchanged + } + oldValue = updated; // we failed, but we have an updated state + } while (true); + } + } internal abstract class CacheItem : CacheItem { + // attempt to get a value that was *not* previously reserved public abstract bool TryGetValue(out T value); - public T GetValue() + // get a value that *was* reserved, countermanding our reservation in the process + public T GetReservedValue() { if (!TryGetValue(out var value)) { Throw(); } + Release(); return value; static void Throw() => throw new ObjectDisposedException("The cache item has been recycled before the value was obtained"); diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.ImmutableCacheItem.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.ImmutableCacheItem.cs index b9138893b84e..2e5d7dc0317a 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.ImmutableCacheItem.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.ImmutableCacheItem.cs @@ -11,13 +11,14 @@ partial class DefaultHybridCache { private sealed class ImmutableCacheItem : CacheItem // used to hold types that do not require defensive copies { - private readonly T _value; - public ImmutableCacheItem(T value) => _value = value; + private T _value = default!; // deferred until SetValue + + public void SetValue(T value) => _value = value; private static ImmutableCacheItem? _sharedDefault; // this is only used when the underlying store is disabled; we don't need 100% singleton; "good enough is" - public static ImmutableCacheItem Default => _sharedDefault ??= new(default!); + public static ImmutableCacheItem Default => _sharedDefault ??= new(); public override bool TryGetValue(out T value) { diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.L2.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.L2.cs index aef445d44e29..578528e9d4be 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.L2.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.L2.cs @@ -113,13 +113,16 @@ private DistributedCacheEntryOptions GetOptions(HybridCacheEntryOptions? options internal void SetL1(string key, CacheItem value, HybridCacheEntryOptions? options) { - // based on CacheExtensions.Set, but with post-eviction recycling - using var cacheEntry = _localCache.CreateEntry(key); - cacheEntry.AbsoluteExpirationRelativeToNow = options?.LocalCacheExpiration ?? _defaultLocalCacheExpiration; - cacheEntry.Value = value; - if (value.NeedsEvictionCallback) + if (value.TryReserve()) // incr ref-count for the the cache itself; this *may* be released via the NeedsEvictionCallback path { - cacheEntry.RegisterPostEvictionCallback(CacheItem._sharedOnEviction); + // based on CacheExtensions.Set, but with post-eviction recycling + using var cacheEntry = _localCache.CreateEntry(key); + cacheEntry.AbsoluteExpirationRelativeToNow = options?.LocalCacheExpiration ?? _defaultLocalCacheExpiration; + cacheEntry.Value = value; + if (value.NeedsEvictionCallback) + { + cacheEntry.RegisterPostEvictionCallback(CacheItem._sharedOnEviction); + } } } } diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.MutableCacheItem.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.MutableCacheItem.cs index 5b1dd6b8cbaf..892883d4a639 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.MutableCacheItem.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.MutableCacheItem.cs @@ -1,28 +1,23 @@ // 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.Diagnostics; -using System.Threading; - namespace Microsoft.Extensions.Caching.Hybrid.Internal; partial class DefaultHybridCache { private sealed partial class MutableCacheItem : CacheItem // used to hold types that require defensive copies { - private readonly IHybridCacheSerializer _serializer; - private readonly BufferChunk _buffer; - private int _refCount = 1; // buffer released when this becomes zero + private IHybridCacheSerializer _serializer = null!; // deferred until SetValue + private BufferChunk _buffer; - public MutableCacheItem(ref BufferChunk buffer, IHybridCacheSerializer serializer) + public void SetValue(ref BufferChunk buffer, IHybridCacheSerializer serializer) { _serializer = serializer; _buffer = buffer; buffer = default; // we're taking over the lifetime; the caller no longer has it! } - public MutableCacheItem(T value, IHybridCacheSerializer serializer, int maxLength) + public void SetValue(T value, IHybridCacheSerializer serializer, int maxLength) { _serializer = serializer; var writer = RecyclableArrayBufferWriter.Create(maxLength); @@ -34,35 +29,10 @@ public MutableCacheItem(T value, IHybridCacheSerializer serializer, int maxLe public override bool NeedsEvictionCallback => _buffer.ReturnToPool; - public override void OnEviction() => Release(); - - public override void Release() + protected override void OnFinalRelease() { - var newCount = Interlocked.Decrement(ref _refCount); - if (newCount == 0) - { - DebugOnlyDecrementOutstandingBuffers(); - _buffer.RecycleIfAppropriate(); - } - } - - public bool TryReserve() - { - var oldValue = Volatile.Read(ref _refCount); - do - { - if (oldValue is 0 or -1) - { - return false; // already burned, or about to roll around back to zero - } - - var updated = Interlocked.CompareExchange(ref _refCount, oldValue + 1, oldValue); - if (updated == oldValue) - { - return true; // we exchanged - } - oldValue = updated; // we failed, but we have an updated state - } while (true); + DebugOnlyDecrementOutstandingBuffers(); + _buffer.RecycleIfAppropriate(); } public override bool TryGetValue(out T value) diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.Stampede.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.Stampede.cs index e975d8a2d922..009d95f0bb08 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.Stampede.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.Stampede.cs @@ -61,16 +61,9 @@ public bool GetOrCreateStampedeState(string key, HybridCacheEntryFlag // and for *them* to have updated needs local-cache-write, but since the shared us/them key includes flags, // we can skip this if *either* flag is set) if ((flags & HybridCacheEntryFlags.DisableLocalCache) == 0 && _localCache.TryGetValue(key, out var untyped) - && untyped is CacheItem typed && typed.TryGetValue(out var value)) + && untyped is CacheItem typed && typed.TryReserve()) { - // set the value against the *current* stampede-state, essentially making it pre-completed; we - // can emulate that by using ImmutableCacheItem (if it isn't already), noting that this state - // will never be in the dictionary, so this value is never shared with anyone else - if (typed is not ImmutableCacheItem immutable) - { - immutable = new ImmutableCacheItem(value); - } - stampedeState.SetResultDirect(immutable); + stampedeState.SetResultDirect(typed); return false; // the work has ALREADY been done } diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeState.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeState.cs index 6a23ea209e64..64385aa54200 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeState.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeState.cs @@ -15,7 +15,7 @@ internal abstract class StampedeState #endif { private readonly DefaultHybridCache _cache; - private int _activeCallers = 1; + private readonly CacheItem _cacheItem; // because multiple callers can enlist, we need to track when the *last* caller cancels // (and keep going until then); that means we need to run with custom cancellation @@ -26,14 +26,16 @@ internal abstract class StampedeState // (both in terms of width and copy-semantics) private readonly StampedeKey _key; public ref readonly StampedeKey Key => ref _key; + protected CacheItem CacheItem => _cacheItem; /// /// Create a stamped token optionally with shared cancellation support /// - protected StampedeState(DefaultHybridCache cache, in StampedeKey key, bool canBeCanceled) + protected StampedeState(DefaultHybridCache cache, in StampedeKey key, CacheItem cacheItem, bool canBeCanceled) { _cache = cache; _key = key; + _cacheItem = cacheItem; if (canBeCanceled) { // if the first (or any) caller can't be cancelled; we'll never get to zero; no point tracking @@ -50,10 +52,11 @@ protected StampedeState(DefaultHybridCache cache, in StampedeKey key, bool canBe /// /// Create a stamped token using a fixed cancellation token /// - protected StampedeState(DefaultHybridCache cache, in StampedeKey key, CancellationToken token) + protected StampedeState(DefaultHybridCache cache, in StampedeKey key, CacheItem cacheItem, CancellationToken token) { _cache = cache; _key = key; + _cacheItem = cacheItem; SharedToken = token; } @@ -71,14 +74,14 @@ protected StampedeState(DefaultHybridCache cache, in StampedeKey key, Cancellati public abstract void SetCanceled(); - public int DebugCallerCount => Volatile.Read(ref _activeCallers); + public int DebugCallerCount => _cacheItem.RefCount; public abstract Type Type { get; } public void RemoveCaller() { // note that TryAddCaller has protections to avoid getting back from zero - if (Interlocked.Decrement(ref _activeCallers) == 0) + if (_cacheItem.Release()) { // we're the last to leave; turn off the lights _sharedCancellation?.Cancel(); @@ -86,24 +89,7 @@ public void RemoveCaller() } } - public bool TryAddCaller() // essentially just interlocked-increment, but with a leading zero check and overflow detection - { - var oldValue = Volatile.Read(ref _activeCallers); - do - { - if (oldValue is 0 or -1) - { - return false; // already burned or about to roll around back to zero - } - - var updated = Interlocked.CompareExchange(ref _activeCallers, oldValue + 1, oldValue); - if (updated == oldValue) - { - return true; // we exchanged - } - oldValue = updated; // we failed, but we have an updated state - } while (true); - } + public bool TryAddCaller() => _cacheItem.TryReserve(); } private void RemoveStampedeState(in StampedeKey key) diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs index 2a58703aaf3a..522b3ec925a0 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs @@ -3,9 +3,9 @@ using System; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Threading; using System.Threading.Tasks; -using Microsoft.Extensions.Caching.Memory; namespace Microsoft.Extensions.Caching.Hybrid.Internal; @@ -19,8 +19,10 @@ internal sealed class StampedeState : StampedeState private HybridCacheEntryOptions? _options; private Task? _sharedUnwrap; // allows multiple non-cancellable callers to share a single task (when no defensive copy needed) + private static CacheItem CreateCacheItem() => ImmutableTypeCache.IsImmutable ? new ImmutableCacheItem() : new MutableCacheItem(); + public StampedeState(DefaultHybridCache cache, in StampedeKey key, bool canBeCanceled) - : base(cache, key, canBeCanceled) + : base(cache, key, CreateCacheItem(), canBeCanceled) { _result = new(TaskCreationOptions.RunContinuationsAsynchronously); } @@ -28,7 +30,7 @@ public StampedeState(DefaultHybridCache cache, in StampedeKey key, bool canBeCan public override Type Type => typeof(T); public StampedeState(DefaultHybridCache cache, in StampedeKey key, CancellationToken token) - : base(cache, key, token) { } // no TCS in this case - this is for SetValue only + : base(cache, key, CreateCacheItem(), token) { } // no TCS in this case - this is for SetValue only public void QueueUserWorkItem(in TState state, Func> underlying, HybridCacheEntryOptions? options) { @@ -174,36 +176,50 @@ private void SetResultAndRecycleIfAppropriate(ref BufferChunk value) var serializer = Cache.GetSerializer(); CacheItem cacheItem; - if (ImmutableTypeCache.IsImmutable) - { - // deserialize; and store object; buffer can be recycled now - cacheItem = new ImmutableCacheItem(serializer.Deserialize(new(value.Array!, 0, value.Length))); - value.RecycleIfAppropriate(); - } - else + switch (CacheItem) { - // use the buffer directly as the backing in the cache-item; do *not* recycle now - var tmp = new MutableCacheItem(ref value, serializer); - tmp.DebugOnlyTrackBuffer(Cache); - cacheItem = tmp; + case ImmutableCacheItem immutable: + // deserialize; and store object; buffer can be recycled now + immutable.SetValue(serializer.Deserialize(new(value.Array!, 0, value.Length))); + value.RecycleIfAppropriate(); + cacheItem = immutable; + break; + case MutableCacheItem mutable: + // use the buffer directly as the backing in the cache-item; do *not* recycle now + mutable.SetValue(ref value, serializer); + mutable.DebugOnlyTrackBuffer(Cache); + cacheItem = mutable; + break; + default: + cacheItem = ThrowUnexpectedCacheItem(); + break; } - SetResult(cacheItem); } + [DoesNotReturn] + static CacheItem ThrowUnexpectedCacheItem() => throw new InvalidOperationException("Unexpected cache item"); + private CacheItem SetResult(T value) { // set a result from a value we calculated directly CacheItem cacheItem; - if (ImmutableTypeCache.IsImmutable) - { - cacheItem = new ImmutableCacheItem(value); // no serialize needed - } - else + switch (CacheItem) { - var tmp = new MutableCacheItem(value, Cache.GetSerializer(), MaximumPayloadBytes); // serialization happens here - tmp.DebugOnlyTrackBuffer(Cache); - cacheItem = tmp; + case ImmutableCacheItem immutable: + // no serialize needed + immutable.SetValue(value); + cacheItem = immutable; + break; + case MutableCacheItem mutable: + // serialization happens here + mutable.SetValue(value, Cache.GetSerializer(), MaximumPayloadBytes); + mutable.DebugOnlyTrackBuffer(Cache); + cacheItem = mutable; + break; + default: + cacheItem = ThrowUnexpectedCacheItem(); + break; } SetResult(cacheItem); return cacheItem; @@ -211,7 +227,7 @@ private CacheItem SetResult(T value) public override void SetCanceled() => _result?.TrySetCanceled(SharedToken); - internal ValueTask UnwrapAsync() + internal ValueTask UnwrapReservedAsync() { var task = Task; #if NETCOREAPP2_0_OR_GREATER || NETSTANDARD2_1_OR_GREATER @@ -220,22 +236,23 @@ internal ValueTask UnwrapAsync() if (task.Status == TaskStatus.RanToCompletion) #endif { - return new(task.Result.GetValue()); + return new(task.Result.GetReservedValue()); } - // if the type is immutable, callers can share the final step too + // if the type is immutable, callers can share the final step too (this may leave dangling + // reservation counters, but that's OK) var result = ImmutableTypeCache.IsImmutable ? (_sharedUnwrap ??= Awaited(Task)) : Awaited(Task); return new(result); static async Task Awaited(Task> task) - => (await task.ConfigureAwait(false)).GetValue(); + => (await task.ConfigureAwait(false)).GetReservedValue(); } public ValueTask JoinAsync(CancellationToken token) { // if the underlying has already completed, and/or our local token can't cancel: we // can simply wrap the shared task; otherwise, we need our own cancellation state - return token.CanBeCanceled && !Task.IsCompleted ? WithCancellation(this, token) : UnwrapAsync(); + return token.CanBeCanceled && !Task.IsCompleted ? WithCancellation(this, token) : UnwrapReservedAsync(); static async ValueTask WithCancellation(StampedeState stampede, CancellationToken token) { @@ -256,7 +273,7 @@ static async ValueTask WithCancellation(StampedeState stampede, Ca Debug.Assert(ReferenceEquals(first, stampede.Task)); // this has already completed, but we'll get the stack nicely - return (await stampede.Task.ConfigureAwait(false)).GetValue(); + return (await stampede.Task.ConfigureAwait(false)).GetReservedValue(); } finally { diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.cs index 947648051cf4..171140c4b722 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.cs @@ -137,7 +137,7 @@ public override ValueTask GetOrCreateAsync(string key, TState stat { // we're going to run to completion; no need to get complicated _ = stampede.ExecuteDirectAsync(in state, underlyingDataCallback, options); // this larger task includes L2 write etc - return stampede.UnwrapAsync(); + return stampede.UnwrapReservedAsync(); } } diff --git a/src/Caching/Hybrid/test/BufferReleaseTests.cs b/src/Caching/Hybrid/test/BufferReleaseTests.cs index 58cd7f3c8fad..5a8c0a8029c7 100644 --- a/src/Caching/Hybrid/test/BufferReleaseTests.cs +++ b/src/Caching/Hybrid/test/BufferReleaseTests.cs @@ -52,6 +52,7 @@ public async Task BufferGetsReleased_NoL2() Assert.NotNull(second); Assert.NotSame(first, second); + Assert.Equal(1, cacheItem.RefCount); await cache.RemoveKeyAsync(key); var third = await cache.GetOrCreateAsync(key, _ => GetAsync(), _noUnderlying); Assert.Null(third); @@ -66,6 +67,7 @@ public async Task BufferGetsReleased_NoL2() #endif // assert that we can *no longer* reserve this buffer, because we've already recycled it Assert.False(cacheItem.TryReserveBuffer(out _)); + Assert.Equal(0, cacheItem.RefCount); Assert.False(cacheItem.NeedsEvictionCallback, "should be recycled now"); static ValueTask GetAsync() => new(new Customer { Id = 42, Name = "Fred" }); } @@ -135,6 +137,7 @@ static bool Write(IBufferWriter destination, byte[]? buffer) Assert.NotNull(second); Assert.NotSame(first, second); + Assert.Equal(1, cacheItem.RefCount); await cache.RemoveKeyAsync(key); var third = await cache.GetOrCreateAsync(key, _ => GetAsync(), _noUnderlying); Assert.Null(third); @@ -151,6 +154,7 @@ static bool Write(IBufferWriter destination, byte[]? buffer) // assert that we can *no longer* reserve this buffer, because we've already recycled it Assert.True(cacheItem.TryReserveBuffer(out _)); // always readable cacheItem.Release(); + Assert.Equal(1, cacheItem.RefCount); // not decremented because there was no need to add the hook Assert.False(cacheItem.NeedsEvictionCallback, "should still not need recycling"); static ValueTask GetAsync() => new(new Customer { Id = 42, Name = "Fred" }); @@ -191,6 +195,7 @@ static bool Write(IBufferWriter destination, byte[]? buffer) Assert.NotNull(second); Assert.NotSame(first, second); + Assert.Equal(1, cacheItem.RefCount); await cache.RemoveKeyAsync(key); var third = await cache.GetOrCreateAsync(key, _ => GetAsync(), _noUnderlying); Assert.Null(third); @@ -206,6 +211,7 @@ static bool Write(IBufferWriter destination, byte[]? buffer) #endif // assert that we can *no longer* reserve this buffer, because we've already recycled it Assert.False(cacheItem.TryReserveBuffer(out _)); // released now + Assert.Equal(0, cacheItem.RefCount); Assert.False(cacheItem.NeedsEvictionCallback, "should be recycled by now"); static ValueTask GetAsync() => new(new Customer { Id = 42, Name = "Fred" }); From 1f2fe6ffb2ad79915057afa59d105dd0e7dfbc3b Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Mon, 22 Apr 2024 12:42:03 +0100 Subject: [PATCH 6/8] add missing accessibility; move a factory method --- .../Hybrid/src/Internal/DefaultHybridCache.CacheItem.cs | 2 ++ .../src/Internal/DefaultHybridCache.StampedeStateT.cs | 8 +++----- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.CacheItem.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.CacheItem.cs index 3893836decc4..bafe45f9f7c2 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.CacheItem.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.CacheItem.cs @@ -73,6 +73,8 @@ public bool TryReserve() internal abstract class CacheItem : CacheItem { + internal static CacheItem Create() => ImmutableTypeCache.IsImmutable ? new ImmutableCacheItem() : new MutableCacheItem(); + // attempt to get a value that was *not* previously reserved public abstract bool TryGetValue(out T value); diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs index 522b3ec925a0..aae92dda00aa 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs @@ -19,10 +19,8 @@ internal sealed class StampedeState : StampedeState private HybridCacheEntryOptions? _options; private Task? _sharedUnwrap; // allows multiple non-cancellable callers to share a single task (when no defensive copy needed) - private static CacheItem CreateCacheItem() => ImmutableTypeCache.IsImmutable ? new ImmutableCacheItem() : new MutableCacheItem(); - public StampedeState(DefaultHybridCache cache, in StampedeKey key, bool canBeCanceled) - : base(cache, key, CreateCacheItem(), canBeCanceled) + : base(cache, key, CacheItem.Create(), canBeCanceled) { _result = new(TaskCreationOptions.RunContinuationsAsynchronously); } @@ -30,7 +28,7 @@ public StampedeState(DefaultHybridCache cache, in StampedeKey key, bool canBeCan public override Type Type => typeof(T); public StampedeState(DefaultHybridCache cache, in StampedeKey key, CancellationToken token) - : base(cache, key, CreateCacheItem(), token) { } // no TCS in this case - this is for SetValue only + : base(cache, key, CacheItem.Create(), token) { } // no TCS in this case - this is for SetValue only public void QueueUserWorkItem(in TState state, Func> underlying, HybridCacheEntryOptions? options) { @@ -198,7 +196,7 @@ private void SetResultAndRecycleIfAppropriate(ref BufferChunk value) } [DoesNotReturn] - static CacheItem ThrowUnexpectedCacheItem() => throw new InvalidOperationException("Unexpected cache item"); + private static CacheItem ThrowUnexpectedCacheItem() => throw new InvalidOperationException("Unexpected cache item"); private CacheItem SetResult(T value) { From 231bed1a610b367df6f4c67a42d9eeb5c73081cc Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Mon, 22 Apr 2024 14:37:43 +0100 Subject: [PATCH 7/8] examples constructed for release notes (to prove they work) --- src/Caching/Hybrid/test/SampleUsage.cs | 200 +++++++++++++++++++++++++ 1 file changed, 200 insertions(+) create mode 100644 src/Caching/Hybrid/test/SampleUsage.cs diff --git a/src/Caching/Hybrid/test/SampleUsage.cs b/src/Caching/Hybrid/test/SampleUsage.cs new file mode 100644 index 000000000000..f2bcc19de6cd --- /dev/null +++ b/src/Caching/Hybrid/test/SampleUsage.cs @@ -0,0 +1,200 @@ +// 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.ComponentModel; +using System.Linq; +using System.Text; +using System.Text.Json; +using System.Threading.Tasks; +using Microsoft.Extensions.Caching.Distributed; +using Microsoft.Extensions.DependencyInjection; + +namespace Microsoft.Extensions.Caching.Hybrid.Tests; + +public class SampleUsage +{ + [Fact] + public async void DistributedCacheWorks() + { + var services = new ServiceCollection(); + services.AddDistributedMemoryCache(); + services.AddTransient(); + using var provider = services.BuildServiceProvider(); + + var obj = provider.GetRequiredService(); + string name = "abc"; + int id = 42; + var x = await obj.GetSomeInformationAsync(name, id); + var y = await obj.GetSomeInformationAsync(name, id); + Assert.NotSame(x, y); + Assert.Equal(id, x.Id); + Assert.Equal(name, x.Name); + Assert.Equal(id, y.Id); + Assert.Equal(name, y.Name); + } + + [Fact] + public async void HybridCacheWorks() + { + var services = new ServiceCollection(); + services.AddHybridCache(); + services.AddTransient(); + using var provider = services.BuildServiceProvider(); + + var obj = provider.GetRequiredService(); + string name = "abc"; + int id = 42; + var x = await obj.GetSomeInformationAsync(name, id); + var y = await obj.GetSomeInformationAsync(name, id); + Assert.NotSame(x, y); + Assert.Equal(id, x.Id); + Assert.Equal(name, x.Name); + Assert.Equal(id, y.Id); + Assert.Equal(name, y.Name); + } + + [Fact] + public async void HybridCacheNoCaptureWorks() + { + var services = new ServiceCollection(); + services.AddHybridCache(); + services.AddTransient(); + using var provider = services.BuildServiceProvider(); + + var obj = provider.GetRequiredService(); + string name = "abc"; + int id = 42; + var x = await obj.GetSomeInformationAsync(name, id); + var y = await obj.GetSomeInformationAsync(name, id); + Assert.NotSame(x, y); + Assert.Equal(id, x.Id); + Assert.Equal(name, x.Name); + Assert.Equal(id, y.Id); + Assert.Equal(name, y.Name); + } + + [Fact] + public async void HybridCacheNoCaptureObjReuseWorks() + { + var services = new ServiceCollection(); + services.AddHybridCache(); + services.AddTransient(); + using var provider = services.BuildServiceProvider(); + + var obj = provider.GetRequiredService(); + string name = "abc"; + int id = 42; + var x = await obj.GetSomeInformationAsync(name, id); + var y = await obj.GetSomeInformationAsync(name, id); + Assert.Same(x, y); + Assert.Equal(id, x.Id); + Assert.Equal(name, x.Name); + } + + public class SomeDCService(IDistributedCache cache) + { + public async Task GetSomeInformationAsync(string name, int id, CancellationToken token = default) + { + var key = $"someinfo:{name}:{id}"; // unique key for this combination + + var bytes = await cache.GetAsync(key, token); // try to get from cache + SomeInformation info; + if (bytes is null) + { + // cache miss; get the data from the real source + info = await SomeExpensiveOperationAsync(name, id, token); + + // serialize and cache it + bytes = SomeSerializer.Serialize(info); + await cache.SetAsync(key, bytes, token); + } + else + { + // cache hit; deserialize it + info = SomeSerializer.Deserialize(bytes); + } + return info; + } + } + + public class SomeHCService(HybridCache cache) + { + public async Task GetSomeInformationAsync(string name, int id, CancellationToken token = default) + { + return await cache.GetOrCreateAsync( + $"someinfo:{name}:{id}", // unique key for this combination + async ct => await SomeExpensiveOperationAsync(name, id, ct), + token: token + ); + } + } + + // this is the work we're trying to cache + private static Task SomeExpensiveOperationAsync(string name, int id, + CancellationToken token = default) + { + return Task.FromResult(new SomeInformation { Id = id, Name = name }); + } + private static Task SomeExpensiveOperationReuseAsync(string name, int id, + CancellationToken token = default) + { + return Task.FromResult(new SomeInformationReuse { Id = id, Name = name }); + } + + public class SomeHCServiceNoCapture(HybridCache cache) + { + public async Task GetSomeInformationAsync(string name, int id, CancellationToken token = default) + { + return await cache.GetOrCreateAsync( + $"someinfo:{name}:{id}", // unique key for this combination + (name, id), // all of the state we need for the final call, if needed + static async (state, token) => + await SomeExpensiveOperationAsync(state.name, state.id, token), + token: token + ); + } + } + + public class SomeHCServiceNoCaptureObjReuse(HybridCache cache, CancellationToken token = default) + { + public async Task GetSomeInformationAsync(string name, int id) + { + return await cache.GetOrCreateAsync( + $"someinfo:{name}:{id}", // unique key for this combination + (name, id), // all of the state we need for the final call, if needed + static async (state, token) => + await SomeExpensiveOperationReuseAsync(state.name, state.id, token), + token: token + ); + } + } + + static class SomeSerializer + { + internal static T Deserialize(byte[] bytes) + { + return JsonSerializer.Deserialize(bytes)!; + } + + internal static byte[] Serialize(T info) + { + using var ms = new MemoryStream(); + JsonSerializer.Serialize(ms, info); + return ms.ToArray(); + } + } + public class SomeInformation + { + public int Id { get; set; } + public string? Name { get; set; } + } + + [ImmutableObject(true)] + public sealed class SomeInformationReuse + { + public int Id { get; set; } + public string? Name { get; set; } + } +} From d8c1d9e11d3edb4ff36d8f30ec56ad5148c5cdc7 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 23 Apr 2024 16:06:04 +0100 Subject: [PATCH 8/8] fix comments for @amcasey (and fix a "oops we *were* going negative, my bad") --- .../Internal/DefaultHybridCache.CacheItem.cs | 5 +++++ .../DefaultHybridCache.ImmutableCacheItem.cs | 19 ++++++++++++++----- .../DefaultHybridCache.StampedeState.cs | 2 +- .../DefaultHybridCache.StampedeStateT.cs | 12 ++++++++---- 4 files changed, 28 insertions(+), 10 deletions(-) diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.CacheItem.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.CacheItem.cs index bafe45f9f7c2..bb3ddee3ef11 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.CacheItem.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.CacheItem.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Diagnostics; using System.Threading; using Microsoft.Extensions.Caching.Memory; @@ -41,6 +42,7 @@ protected virtual void OnFinalRelease() { } // any required release semantics public bool Release() // returns true ONLY for the final release step { var newCount = Interlocked.Decrement(ref _refCount); + Debug.Assert(newCount >= 0, "over-release detected"); if (newCount == 0) { // perform per-item clean-up, i.e. buffer recycling (if defensive copies needed) @@ -52,6 +54,9 @@ protected virtual void OnFinalRelease() { } // any required release semantics public bool TryReserve() { + // this is basically interlocked increment, but with a check against: + // a) incrementing upwards from zero + // b) overflowing *back* to zero var oldValue = Volatile.Read(ref _refCount); do { diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.ImmutableCacheItem.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.ImmutableCacheItem.cs index 2e5d7dc0317a..dd245912f6ee 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.ImmutableCacheItem.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.ImmutableCacheItem.cs @@ -1,9 +1,7 @@ // 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.Diagnostics; -using System.Diagnostics.CodeAnalysis; +using System.Threading; namespace Microsoft.Extensions.Caching.Hybrid.Internal; @@ -17,8 +15,19 @@ partial class DefaultHybridCache private static ImmutableCacheItem? _sharedDefault; - // this is only used when the underlying store is disabled; we don't need 100% singleton; "good enough is" - public static ImmutableCacheItem Default => _sharedDefault ??= new(); + // get a shared instance that passes as "reserved"; doesn't need to be 100% singleton, + // but we don't want to break the reservation rules either; if we can't reserve: create new + public static ImmutableCacheItem GetReservedShared() + { + var obj = Volatile.Read(ref _sharedDefault); + if (obj is null || !obj.TryReserve()) + { + obj = new(); + obj.TryReserve(); // this is reliable on a new instance + Volatile.Write(ref _sharedDefault, obj); + } + return obj; + } public override bool TryGetValue(out T value) { diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeState.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeState.cs index 64385aa54200..c59378c3df0d 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeState.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeState.cs @@ -78,7 +78,7 @@ protected StampedeState(DefaultHybridCache cache, in StampedeKey key, CacheItem public abstract Type Type { get; } - public void RemoveCaller() + public void CancelCaller() { // note that TryAddCaller has protections to avoid getting back from zero if (_cacheItem.Release()) diff --git a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs index aae92dda00aa..b88ed5f236b7 100644 --- a/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs +++ b/src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs @@ -163,7 +163,7 @@ private void SetDefaultResult() if (_result is not null) { Cache.RemoveStampedeState(in Key); - _result.TrySetResult(ImmutableCacheItem.Default); + _result.TrySetResult(ImmutableCacheItem.GetReservedShared()); } } @@ -260,6 +260,7 @@ static async ValueTask WithCancellation(StampedeState stampede, Ca ((TaskCompletionSource)obj!).TrySetResult(true); }, cancelStub); + CacheItem result; try { var first = await System.Threading.Tasks.Task.WhenAny(stampede.Task, cancelStub.Task).ConfigureAwait(false); @@ -271,12 +272,15 @@ static async ValueTask WithCancellation(StampedeState stampede, Ca Debug.Assert(ReferenceEquals(first, stampede.Task)); // this has already completed, but we'll get the stack nicely - return (await stampede.Task.ConfigureAwait(false)).GetReservedValue(); + result = await stampede.Task.ConfigureAwait(false); } - finally + catch { - stampede.RemoveCaller(); + stampede.CancelCaller(); + throw; } + // outside the catch, so we know we only decrement one way or the other + return result.GetReservedValue(); } } }