diff --git a/BitFaster.Caching.UnitTests/Atomic/AtomicFactoryCacheTests.cs b/BitFaster.Caching.UnitTests/Atomic/AtomicFactoryCacheTests.cs index f4ab92cb..8c047ec6 100644 --- a/BitFaster.Caching.UnitTests/Atomic/AtomicFactoryCacheTests.cs +++ b/BitFaster.Caching.UnitTests/Atomic/AtomicFactoryCacheTests.cs @@ -13,11 +13,18 @@ namespace BitFaster.Caching.UnitTests.Atomic public class AtomicFactoryCacheTests { private const int capacity = 6; - private readonly AtomicFactoryCache cache = new(new ConcurrentLru>(capacity)); + private readonly ConcurrentLru> innerCache; + private readonly AtomicFactoryCache cache; private List> removedItems = new(); private List> updatedItems = new(); + public AtomicFactoryCacheTests() + { + innerCache = new ConcurrentLru>(capacity); + cache = new(innerCache); + } + [Fact] public void WhenInnerCacheIsNullCtorThrows() { @@ -70,10 +77,52 @@ public void WhenRemovedEventHandlerIsRegisteredItIsFired() this.cache.TryRemove(1); this.removedItems.First().Key.Should().Be(1); + } + + // backcompat: remove conditional compile +#if NETCOREAPP3_0_OR_GREATER + [Fact] + public void WhenRemovedValueIsReturned() + { + this.cache.AddOrUpdate(1, 1); + this.cache.TryRemove(1, out var value); + + value.Should().Be(1); + } + + [Fact] + public void WhenNotRemovedValueIsDefault() + { + this.cache.AddOrUpdate(1, 1); + this.cache.TryRemove(2, out var value); + + value.Should().Be(0); + } + + [Fact] + public void WhenRemoveKeyValueAndValueDoesntMatchDontRemove() + { + this.cache.AddOrUpdate(1, 1); + this.cache.TryRemove(new KeyValuePair(1, 2)).Should().BeFalse(); + } + + [Fact] + public void WhenRemoveKeyValueAndValueDoesMatchThenRemove() + { + this.cache.AddOrUpdate(1, 1); + this.cache.TryRemove(new KeyValuePair(1, 1)).Should().BeTrue(); + } + + [Fact] + public void WhenRemoveKeyValueAndValueIsNotCreatedDoesNotRemove() + { + // seed the inner cache with an not yet created value + this.innerCache.AddOrUpdate(1, new AtomicFactory()); + + // try to remove with the default value (0) + this.cache.TryRemove(new KeyValuePair(1, 0)).Should().BeFalse(); } -// backcompat: remove conditional compile -#if NETCOREAPP3_0_OR_GREATER [Fact] public void WhenUpdatedEventHandlerIsRegisteredItIsFired() { diff --git a/BitFaster.Caching.UnitTests/Atomic/AtomicFactoryTests.cs b/BitFaster.Caching.UnitTests/Atomic/AtomicFactoryTests.cs index d3c72caf..85987dfe 100644 --- a/BitFaster.Caching.UnitTests/Atomic/AtomicFactoryTests.cs +++ b/BitFaster.Caching.UnitTests/Atomic/AtomicFactoryTests.cs @@ -63,6 +63,58 @@ public void WhenValueCreatedArgGetValueReturnsOriginalValue() a.GetValue(1, (k, a) => k + a, 9).Should().Be(8); } + [Fact] + public void WhenValueNotCreatedHashCodeIsZero() + { + new AtomicFactory() + .GetHashCode() + .Should().Be(0); + } + + [Fact] + public void WhenValueCreatedHashCodeIsValueHashCode() + { + new AtomicFactory(1) + .GetHashCode() + .Should().Be(1); + } + + [Fact] + public void WhenValueNotCreatedEqualsFalse() + { + var a = new AtomicFactory(); + var b = new AtomicFactory(); + + a.Equals(b).Should().BeFalse(); + } + + [Fact] + public void WhenOtherValueNotCreatedEqualsFalse() + { + var a = new AtomicFactory(1); + var b = new AtomicFactory(); + + a.Equals(b).Should().BeFalse(); + } + + [Fact] + public void WhenArgNullEqualsFalse() + { + new AtomicFactory(1) + .Equals(null) + .Should().BeFalse(); + } + + [Fact] + public void WhenArgObjectValuesAreSameEqualsTrue() + { + object other = new AtomicFactory(1); + + new AtomicFactory(1) + .Equals(other) + .Should().BeTrue(); + } + [Fact] public async Task WhenCallersRunConcurrentlyResultIsFromWinner() { diff --git a/BitFaster.Caching.UnitTests/CacheTests.cs b/BitFaster.Caching.UnitTests/CacheTests.cs index b9ae6b5f..1840e301 100644 --- a/BitFaster.Caching.UnitTests/CacheTests.cs +++ b/BitFaster.Caching.UnitTests/CacheTests.cs @@ -1,5 +1,6 @@  using System; +using System.Collections.Generic; using System.Threading.Tasks; using FluentAssertions; using Moq; @@ -25,6 +26,28 @@ public void WhenCacheInterfaceDefaultGetOrAddFallback() 1, (k, a) => k + a, 2).Should().Be(3); + } + + [Fact] + public void WhenCacheInterfaceDefaultTryRemoveKeyThrows() + { + var cache = new Mock>(); + cache.CallBase = true; + + Action tryRemove = () => { cache.Object.TryRemove(1, out var value); }; + + tryRemove.Should().Throw(); + } + + [Fact] + public void WhenCacheInterfaceDefaultTryRemoveKeyValueThrows() + { + var cache = new Mock>(); + cache.CallBase = true; + + Action tryRemove = () => { cache.Object.TryRemove(new KeyValuePair(1, 1)); }; + + tryRemove.Should().Throw(); } [Fact] diff --git a/BitFaster.Caching.UnitTests/Lfu/ConcurrentLfuTests.cs b/BitFaster.Caching.UnitTests/Lfu/ConcurrentLfuTests.cs index 73e9c762..453a77a6 100644 --- a/BitFaster.Caching.UnitTests/Lfu/ConcurrentLfuTests.cs +++ b/BitFaster.Caching.UnitTests/Lfu/ConcurrentLfuTests.cs @@ -582,12 +582,39 @@ public void WhenItemDoesNotExistUpdatedAddsItem() } [Fact] - public void WhenItemIsRemovedItIsRemoved() + public void WhenKeyExistsTryRemoveRemovesItem() { cache.GetOrAdd(1, k => k); cache.TryRemove(1).Should().BeTrue(); - cache.TryGet(1, out var value).Should().BeFalse(); + cache.TryGet(1, out _).Should().BeFalse(); + } + + [Fact] + public void WhenKeyExistsTryRemoveReturnsValue() + { + cache.GetOrAdd(1, valueFactory.Create); + + cache.TryRemove(1, out var value).Should().BeTrue(); + value.Should().Be(1); + } + + [Fact] + public void WhenItemExistsTryRemoveRemovesItem() + { + cache.GetOrAdd(1, k => k); + + cache.TryRemove(new KeyValuePair(1, 1)).Should().BeTrue(); + cache.TryGet(1, out _).Should().BeFalse(); + } + + [Fact] + public void WhenItemDoesntMatchTryRemoveDoesNotRemove() + { + cache.GetOrAdd(1, k => k); + + cache.TryRemove(new KeyValuePair(1, 2)).Should().BeFalse(); + cache.TryGet(1, out var value).Should().BeTrue(); } [Fact] diff --git a/BitFaster.Caching.UnitTests/Lru/ClassicLruTests.cs b/BitFaster.Caching.UnitTests/Lru/ClassicLruTests.cs index def8a864..9141759b 100644 --- a/BitFaster.Caching.UnitTests/Lru/ClassicLruTests.cs +++ b/BitFaster.Caching.UnitTests/Lru/ClassicLruTests.cs @@ -362,6 +362,33 @@ public void WhenKeyExistsTryRemoveRemovesItemAndReturnsTrue() lru.TryGet(1, out var value).Should().BeFalse(); } + [Fact] + public void WhenKeyExistsTryRemoveReturnsValue() + { + lru.GetOrAdd(1, valueFactory.Create); + + lru.TryRemove(1, out var value).Should().BeTrue(); + value.Should().Be("1"); + } + + [Fact] + public void WhenItemExistsTryRemovesItemAndReturnsTrue() + { + lru.GetOrAdd(1, valueFactory.Create); + + lru.TryRemove(new KeyValuePair(1, "1")).Should().BeTrue(); + lru.TryGet(1, out var value).Should().BeFalse(); + } + + [Fact] + public void WhenTryRemoveKvpDoesntMatchItemNotRemovedAndReturnsFalse() + { + lru.GetOrAdd(1, valueFactory.Create); + + lru.TryRemove(new KeyValuePair(1, "2")).Should().BeFalse(); + lru.TryGet(1, out var value).Should().BeTrue(); + } + [Fact] public void WhenItemIsRemovedItIsDisposed() { diff --git a/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs b/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs index ed2bf960..74f7800f 100644 --- a/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs +++ b/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs @@ -634,6 +634,15 @@ public void WhenKeyExistsTryRemoveRemovesItemAndReturnsTrue() lru.TryRemove(1).Should().BeTrue(); lru.TryGet(1, out var value).Should().BeFalse(); + } + + [Fact] + public void WhenKeyExistsTryRemoveReturnsValue() + { + lru.GetOrAdd(1, valueFactory.Create); + + lru.TryRemove(1, out var value).Should().BeTrue(); + value.Should().Be("1"); } [Fact] @@ -1196,6 +1205,26 @@ await Threaded.Run(4, () => { } } + [Fact] + public async Task WhenSoakConcurrentGetAndRemoveKvpCacheEndsInConsistentState() + { + for (int i = 0; i < 10; i++) + { + await Threaded.Run(4, () => { + for (int i = 0; i < 100000; i++) + { + lru.TryRemove(new KeyValuePair(i + 1, (i + 1).ToString())); + lru.GetOrAdd(i + 1, i => i.ToString()); + } + }); + + this.testOutputHelper.WriteLine($"{lru.HotCount} {lru.WarmCount} {lru.ColdCount}"); + this.testOutputHelper.WriteLine(string.Join(" ", lru.Keys)); + + RunIntegrityCheck(); + } + } + [Fact] public async Task WhenSoakConcurrentGetAndUpdateCacheEndsInConsistentState() { diff --git a/BitFaster.Caching/Atomic/AtomicFactory.cs b/BitFaster.Caching/Atomic/AtomicFactory.cs index aeb96a67..d2f93677 100644 --- a/BitFaster.Caching/Atomic/AtomicFactory.cs +++ b/BitFaster.Caching/Atomic/AtomicFactory.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Diagnostics; using System.Threading; @@ -11,7 +12,7 @@ namespace BitFaster.Caching.Atomic /// The type of the key. /// The type of the value. [DebuggerDisplay("IsValueCreated={IsValueCreated}, Value={ValueIfCreated}")] - public sealed class AtomicFactory + public sealed class AtomicFactory : IEquatable> { private Initializer initializer; @@ -102,8 +103,36 @@ private V CreateValue(K key, TFactory valueFactory) where TFactory : s } return value; - } - + } + + /// + public override bool Equals(object obj) + { + return Equals(obj as AtomicFactory); + } + + /// + public bool Equals(AtomicFactory other) + { + if (other is null || !IsValueCreated || !other.IsValueCreated) + { + return false; + } + + return EqualityComparer.Default.Equals(ValueIfCreated, other.ValueIfCreated); + } + + /// + public override int GetHashCode() + { + if (!IsValueCreated) + { + return 0; + } + + return ValueIfCreated.GetHashCode(); + } + private class Initializer { private readonly object syncLock = new(); @@ -129,6 +158,6 @@ public V CreateValue(K key, TFactory valueFactory) where TFactory : st return value; } } - } + } } } diff --git a/BitFaster.Caching/Atomic/AtomicFactoryCache.cs b/BitFaster.Caching/Atomic/AtomicFactoryCache.cs index f814ee80..6af07f8c 100644 --- a/BitFaster.Caching/Atomic/AtomicFactoryCache.cs +++ b/BitFaster.Caching/Atomic/AtomicFactoryCache.cs @@ -2,7 +2,8 @@ using System.Collections; using System.Collections.Generic; using System.Diagnostics; - +using System.Linq.Expressions; + namespace BitFaster.Caching.Atomic { /// @@ -100,9 +101,38 @@ public bool TryGet(K key, out V value) return true; } + value = default; + return false; + } + + // backcompat: remove conditional compile +#if NETCOREAPP3_0_OR_GREATER + /// + /// + ///If the value factory is still executing, returns false. + /// + public bool TryRemove(KeyValuePair item) + { + var kvp = new KeyValuePair>(item.Key, new AtomicFactory(item.Value)); + return cache.TryRemove(kvp); + } + + /// + /// + /// If the value factory is still executing, the default value will be returned. + /// + public bool TryRemove(K key, out V value) + { + if (cache.TryRemove(key, out var atomic)) + { + value = atomic.ValueIfCreated; + return true; + } + value = default; return false; } +#endif /// public bool TryRemove(K key) diff --git a/BitFaster.Caching/ICache.cs b/BitFaster.Caching/ICache.cs index 33ff16de..00e16c96 100644 --- a/BitFaster.Caching/ICache.cs +++ b/BitFaster.Caching/ICache.cs @@ -66,6 +66,21 @@ public interface ICache : IEnumerable> /// The value for the key. This will be either the existing value for the key if the key is already /// in the cache, or the new value if the key was not in the cache. V GetOrAdd(K key, Func valueFactory, TArg factoryArgument) => this.GetOrAdd(key, k => valueFactory(k, factoryArgument)); + + /// + /// Attempts to remove and return the value that has the specified key. + /// + /// The key of the element to remove. + /// When this method returns, contains the object removed, or the default value of the value type if key does not exist. + /// true if the object was removed successfully; otherwise, false. + bool TryRemove(K key, out V value) => throw new NotSupportedException(); + + /// + /// Attempts to remove the specified key value pair. + /// + /// The item to remove. + /// true if the item was removed successfully; otherwise, false. + bool TryRemove(KeyValuePair item) => throw new NotSupportedException(); #endif /// diff --git a/BitFaster.Caching/Lfu/ConcurrentLfu.cs b/BitFaster.Caching/Lfu/ConcurrentLfu.cs index 223735ba..5d4a3bc7 100644 --- a/BitFaster.Caching/Lfu/ConcurrentLfu.cs +++ b/BitFaster.Caching/Lfu/ConcurrentLfu.cs @@ -316,21 +316,64 @@ public bool TryGet(K key, out V value) value = default; return false; - } + } + + /// + /// Attempts to remove the specified key value pair. + /// + /// The item to remove. + /// true if the item was removed successfully; otherwise, false. + public bool TryRemove(KeyValuePair item) + { + if (this.dictionary.TryGetValue(item.Key, out var node)) + { + if (EqualityComparer.Default.Equals(node.Value, item.Value)) + { + var kvp = new KeyValuePair>(item.Key, node); + +#if NET6_0_OR_GREATER + if (this.dictionary.TryRemove(kvp)) +#else + // https://devblogs.microsoft.com/pfxteam/little-known-gems-atomic-conditional-removals-from-concurrentdictionary/ + if (((ICollection>>)this.dictionary).Remove(kvp)) +#endif + { + node.WasRemoved = true; + AfterWrite(node); + return true; + } + } + } - /// - public bool TryRemove(K key) + return false; + } + + /// + /// Attempts to remove and return the value that has the specified key. + /// + /// The key of the element to remove. + /// When this method returns, contains the object removed, or the default value of the value type if key does not exist. + /// true if the object was removed successfully; otherwise, false. + public bool TryRemove(K key, out V value) { if (this.dictionary.TryRemove(key, out var node)) { node.WasRemoved = true; AfterWrite(node); + value = node.Value; return true; } + value = default; return false; } + /// + public bool TryRemove(K key) + { + return this.TryRemove(key, out var _); + } + /// public bool TryUpdate(K key, V value) { diff --git a/BitFaster.Caching/Lru/ClassicLru.cs b/BitFaster.Caching/Lru/ClassicLru.cs index 5950bb93..1b777691 100644 --- a/BitFaster.Caching/Lru/ClassicLru.cs +++ b/BitFaster.Caching/Lru/ClassicLru.cs @@ -235,33 +235,80 @@ public async ValueTask GetOrAddAsync(K key, Func> valu } return await this.GetOrAddAsync(key, valueFactory, factoryArgument); - } - - /// - public bool TryRemove(K key) + } + + /// + /// Attempts to remove the specified key value pair. + /// + /// The item to remove. + /// true if the item was removed successfully; otherwise, false. + public bool TryRemove(KeyValuePair item) + { + if (this.dictionary.TryGetValue(item.Key, out var node)) + { + if (EqualityComparer.Default.Equals(node.Value.Value, item.Value)) + { + var kvp = new KeyValuePair>(item.Key, node); + +#if NET6_0_OR_GREATER + if (this.dictionary.TryRemove(kvp)) +#else + // https://devblogs.microsoft.com/pfxteam/little-known-gems-atomic-conditional-removals-from-concurrentdictionary/ + if (((ICollection>>)this.dictionary).Remove(kvp)) +#endif + { + OnRemove(node); + return true; + } + } + } + + return false; + } + + /// + /// Attempts to remove and return the value that has the specified key. + /// + /// The key of the element to remove. + /// When this method returns, contains the object removed, or the default value of the value type if key does not exist. + /// true if the object was removed successfully; otherwise, false. + public bool TryRemove(K key, out V value) { if (dictionary.TryRemove(key, out var node)) { - // If the node has already been removed from the list, ignore. - // E.g. thread A reads x from the dictionary. Thread B adds a new item, removes x from - // the List & Dictionary. Now thread A will try to move x to the end of the list. - if (node.List != null) - { - lock (this.linkedList) - { - if (node.List != null) - { - linkedList.Remove(node); - } - } - } - - Disposer.Dispose(node.Value.Value); - + OnRemove(node); + value = node.Value.Value; return true; - } + } + + value = default; + return false; + + } - return false; + /// + public bool TryRemove(K key) + { + return TryRemove(key, out var _); + } + + private void OnRemove(LinkedListNode node) + { + // If the node has already been removed from the list, ignore. + // E.g. thread A reads x from the dictionary. Thread B adds a new item, removes x from + // the List & Dictionary. Now thread A will try to move x to the end of the list. + if (node.List != null) + { + lock (this.linkedList) + { + if (node.List != null) + { + linkedList.Remove(node); + } + } + } + + Disposer.Dispose(node.Value.Value); } /// diff --git a/BitFaster.Caching/Lru/ConcurrentLruCore.cs b/BitFaster.Caching/Lru/ConcurrentLruCore.cs index 4ee2eab6..59ec796c 100644 --- a/BitFaster.Caching/Lru/ConcurrentLruCore.cs +++ b/BitFaster.Caching/Lru/ConcurrentLruCore.cs @@ -293,45 +293,79 @@ public async ValueTask GetOrAddAsync(K key, Func> valu } } - /// - public bool TryRemove(K key) + /// + /// Attempts to remove the specified key value pair. + /// + /// The item to remove. + /// true if the item was removed successfully; otherwise, false. + public bool TryRemove(KeyValuePair item) { - while (true) - { - if (this.dictionary.TryGetValue(key, out var existing)) + if (this.dictionary.TryGetValue(item.Key, out var existing)) + { + if (EqualityComparer.Default.Equals(existing.Value, item.Value)) { - var kvp = new KeyValuePair(key, existing); - - // hidden atomic remove + var kvp = new KeyValuePair(item.Key, existing); +#if NET6_0_OR_GREATER + if (this.dictionary.TryRemove(kvp)) +#else // https://devblogs.microsoft.com/pfxteam/little-known-gems-atomic-conditional-removals-from-concurrentdictionary/ if (((ICollection>)this.dictionary).Remove(kvp)) +#endif { - // Mark as not accessed, it will later be cycled out of the queues because it can never be fetched - // from the dictionary. Note: Hot/Warm/Cold count will reflect the removed item until it is cycled - // from the queue. - existing.WasAccessed = false; - existing.WasRemoved = true; + OnRemove(item.Key, kvp.Value); + return true; + } + } - this.telemetryPolicy.OnItemRemoved(existing.Key, existing.Value, ItemRemovedReason.Removed); + // it existed, but we couldn't remove - this means value was replaced afer the TryGetValue (a race) + } - // serialize dispose (common case dispose not thread safe) - lock (existing) - { - Disposer.Dispose(existing.Value); - } + return false; + } - return true; - } + /// + /// Attempts to remove and return the value that has the specified key. + /// + /// The key of the element to remove. + /// When this method returns, contains the object removed, or the default value of the value type if key does not exist. + /// true if the object was removed successfully; otherwise, false. + public bool TryRemove(K key, out V value) + { + if (this.dictionary.TryRemove(key, out var item)) + { + OnRemove(key, item); + value = item.Value; + return true; + } - // it existed, but we couldn't remove - this means value was replaced afer the TryGetValue (a race), try again - } - else - { - return false; - } + value = default; + return false; + } + + /// + public bool TryRemove(K key) + { + return TryRemove(key, out _); + } + + private void OnRemove(K key, I item) + { + // Mark as not accessed, it will later be cycled out of the queues because it can never be fetched + // from the dictionary. Note: Hot/Warm/Cold count will reflect the removed item until it is cycled + // from the queue. + item.WasAccessed = false; + item.WasRemoved = true; + + this.telemetryPolicy.OnItemRemoved(key, item.Value, ItemRemovedReason.Removed); + + // serialize dispose (common case dispose not thread safe) + lock (item.Value) + { + Disposer.Dispose(item.Value); } } + /// ///Note: Calling this method does not affect LRU order. public bool TryUpdate(K key, V value)