diff --git a/BitFaster.Caching.UnitTests/Lru/ConcurrentLruSoakTests.cs b/BitFaster.Caching.UnitTests/Lru/ConcurrentLruSoakTests.cs index d78262c9..aa1f13c6 100644 --- a/BitFaster.Caching.UnitTests/Lru/ConcurrentLruSoakTests.cs +++ b/BitFaster.Caching.UnitTests/Lru/ConcurrentLruSoakTests.cs @@ -1,4 +1,5 @@ -using System.Collections.Generic; +using System; +using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; using BitFaster.Caching.Lru; @@ -151,7 +152,7 @@ await Threaded.Run(4, () => { } [Fact] - public async Task WhenSoakConcurrentGetAndUpdateCacheEndsInConsistentState() + public async Task WhenSoakConcurrentGetAndUpdateRefTypeCacheEndsInConsistentState() { for (int i = 0; i < 10; i++) { @@ -170,6 +171,29 @@ await Threaded.Run(4, () => { } } + [Fact] + public async Task WhenSoakConcurrentGetAndUpdateValueTypeCacheEndsInConsistentState() + { + var lruVT = new ConcurrentLru(1, capacity, EqualityComparer.Default); + + for (int i = 0; i < 10; i++) + { + await Threaded.Run(4, () => { + var b = new byte[8]; + for (int i = 0; i < 100000; i++) + { + lruVT.TryUpdate(i + 1, new Guid(i, 0, 0, b)); + lruVT.GetOrAdd(i + 1, x => new Guid(x, 0, 0, b)); + } + }); + + this.testOutputHelper.WriteLine($"{lruVT.HotCount} {lruVT.WarmCount} {lruVT.ColdCount}"); + this.testOutputHelper.WriteLine(string.Join(" ", lruVT.Keys)); + + new ConcurrentLruIntegrityChecker, LruPolicy, TelemetryPolicy>(lruVT).Validate(); + } + } + [Fact] public async Task WhenSoakConcurrentGetAndAddCacheEndsInConsistentState() { diff --git a/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs b/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs index 1a9b32f1..4dd80708 100644 --- a/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs +++ b/BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs @@ -711,6 +711,22 @@ public void WhenKeyExistsTryUpdateUpdatesValueAndReturnsTrue() value.Should().Be("2"); } + [Fact] + public void WhenKeyExistsTryUpdateGuidUpdatesValueAndReturnsTrue() + { + var lru2 = new ConcurrentLru(1, capacity, EqualityComparer.Default); + var b = new byte[8]; + + lru2.GetOrAdd(1, x => new Guid(x, 0, 0, b)); + + lru2.TryUpdate(1, new Guid(2, 0, 0, b)).Should().BeTrue(); + + lru2.TryGet(1, out var value); + value.Should().Be(new Guid(2, 0, 0, b)); + + new ConcurrentLruIntegrityChecker, LruPolicy, TelemetryPolicy>(lru2).Validate(); + } + [Fact] public void WhenKeyExistsTryUpdateDisposesOldValue() { @@ -1357,7 +1373,7 @@ private void ValidateQueue(ConcurrentLruCore cache, ConcurrentQue } else { - dictionary.TryGetValue(item.Key, out var value).Should().BeTrue($"{queueName} item {item.Key} was not present"); + dictionary.TryGetValue(item.Key, out var value).Should().BeTrue($"{queueName} item {item.Key} is not marked as removed"); } } } diff --git a/BitFaster.Caching.UnitTests/Lru/LongTickCountLruItemTests.cs b/BitFaster.Caching.UnitTests/Lru/LongTickCountLruItemTests.cs new file mode 100644 index 00000000..30ce870a --- /dev/null +++ b/BitFaster.Caching.UnitTests/Lru/LongTickCountLruItemTests.cs @@ -0,0 +1,24 @@ +using System.Collections.Concurrent; +using BitFaster.Caching.Lru; +using FluentAssertions; +using Xunit; + +namespace BitFaster.Caching.UnitTests.Lru +{ + public class LongTickCountLruItemTests + { + // Validate that using the base class Equals/HashCode we can update ConcurrentDictionary + // This replicates ConcurrentLruCore.TryUpdate for non-write atomic updates. + [Fact] + public void WhenInConcurrentDictionaryCanBeReplaced() + { + var item1 = new LongTickCountLruItem(1, 2, 3); + var item2 = new LongTickCountLruItem(2, 1, 0); + + var d = new ConcurrentDictionary>(); + d.TryAdd(1, item1); + + d.TryUpdate(1, item2, item1).Should().BeTrue(); + } + } +} diff --git a/BitFaster.Caching.UnitTests/Lru/LruItemTests.cs b/BitFaster.Caching.UnitTests/Lru/LruItemTests.cs new file mode 100644 index 00000000..fbdd833f --- /dev/null +++ b/BitFaster.Caching.UnitTests/Lru/LruItemTests.cs @@ -0,0 +1,51 @@ +using BitFaster.Caching.Lru; +using FluentAssertions; +using Xunit; + +namespace BitFaster.Caching.UnitTests.Lru +{ + public class LruItemTests + { + [Fact] + public void EqualsWithSameReferenceReturnsTrue() + { + var item = new LruItem(1, 2); + + item.Equals(item).Should().BeTrue(); + } + + [Fact] + public void EqualsObjectWithSameReferenceReturnsTrue() + { + var item = new LruItem(1, 2); + + item.Equals((object)item).Should().BeTrue(); + } + + [Fact] + public void EqualsWithSameValuesReturnsFalse() + { + var item1 = new LruItem(1, 2); + var item2 = new LruItem(1, 2); + + // this is used for CAS algorithms, so this must be false + item1.Equals(item2).Should().BeFalse(); + } + + [Fact] + public void GetHashCodeWithDifferentObjectsIsDifferent() + { + var item1 = new LruItem(1, 2); + var item2 = new LruItem(2, 1); + + item1.GetHashCode().Should().NotBe(item2.GetHashCode()); + } + + [Fact] + public void GetHashCodeHandlesNulls() + { + var item1 = new LruItem(null, null); + item1.GetHashCode().Should().NotBe(0); + } + } +} diff --git a/BitFaster.Caching.UnitTests/TypePropsTests.cs b/BitFaster.Caching.UnitTests/TypePropsTests.cs new file mode 100644 index 00000000..a02d9c80 --- /dev/null +++ b/BitFaster.Caching.UnitTests/TypePropsTests.cs @@ -0,0 +1,31 @@ +using System; +using System.Reflection; +using FluentAssertions; +using Xunit; + +namespace BitFaster.Caching.UnitTests +{ + public class TypePropsTests + { + private static readonly MethodInfo method = typeof(TypePropsTests).GetMethod(nameof(TypePropsTests.IsWriteAtomic), BindingFlags.NonPublic | BindingFlags.Static); + + [Theory] + [InlineData(typeof(object), true)] + [InlineData(typeof(IntPtr), true)] + [InlineData(typeof(UIntPtr), true)] + [InlineData(typeof(int), true)] + [InlineData(typeof(long), true)] // this is only expected to pass on 64bit platforms + [InlineData(typeof(Guid), false)] + public void Test(Type argType, bool expected) + { + var isWriteAtomic = method.MakeGenericMethod(argType); + + isWriteAtomic.Invoke(null, null).Should().BeOfType().Which.Should().Be(expected); + } + + private static bool IsWriteAtomic() + { + return TypeProps.IsWriteAtomic; + } + } +} diff --git a/BitFaster.Caching/Lru/ConcurrentLruCore.cs b/BitFaster.Caching/Lru/ConcurrentLruCore.cs index 0438ffd1..4bab79e6 100644 --- a/BitFaster.Caching/Lru/ConcurrentLruCore.cs +++ b/BitFaster.Caching/Lru/ConcurrentLruCore.cs @@ -382,11 +382,35 @@ public bool TryUpdate(K key, V value) if (!existing.WasRemoved) { V oldValue = existing.Value; - existing.Value = value; - this.itemPolicy.Update(existing); + + if (TypeProps.IsWriteAtomic) + { + existing.Value = value; + this.itemPolicy.Update(existing); + } + else + { + // we can't safely update value in place so we need a new item + var newItem = this.itemPolicy.CreateItem(key, value); + + // update the dictionary to the new item + if (!this.dictionary.TryUpdate(key, newItem, existing)) + { + return false; + } + + // we cannot swap items within the queue, so mark existing as removed and enqueue the new item + existing.WasAccessed = false; + existing.WasRemoved = true; + this.hotQueue.Enqueue(newItem); + Cycle(Interlocked.Increment(ref counter.hot)); + + this.itemPolicy.Update(newItem); + } + // backcompat: remove conditional compile #if NETCOREAPP3_0_OR_GREATER - this.telemetryPolicy.OnItemUpdated(existing.Key, oldValue, existing.Value); + this.telemetryPolicy.OnItemUpdated(existing.Key, oldValue, value); #endif Disposer.Dispose(oldValue); diff --git a/BitFaster.Caching/Lru/LongTickCountLruItem.cs b/BitFaster.Caching/Lru/LongTickCountLruItem.cs index 31de3feb..a4eeda0c 100644 --- a/BitFaster.Caching/Lru/LongTickCountLruItem.cs +++ b/BitFaster.Caching/Lru/LongTickCountLruItem.cs @@ -1,28 +1,28 @@ - -namespace BitFaster.Caching.Lru -{ - /// - /// Represents an LRU item that also stores tick count. - /// - /// The type of the key. - /// The type of the value. - public class LongTickCountLruItem : LruItem - { - /// - /// Initializes a new instance of the LongTickCountLruItem class with the specified key and value. - /// - /// The key. - /// The value. - /// The tick count. - public LongTickCountLruItem(K key, V value, long tickCount) - : base(key, value) - { - this.TickCount = tickCount; - } - - /// - /// Gets or sets the tick count. - /// - public long TickCount { get; set; } - } -} + +namespace BitFaster.Caching.Lru +{ + /// + /// Represents an LRU item that also stores tick count. + /// + /// The type of the key. + /// The type of the value. + public class LongTickCountLruItem : LruItem + { + /// + /// Initializes a new instance of the LongTickCountLruItem class with the specified key and value. + /// + /// The key. + /// The value. + /// The tick count. + public LongTickCountLruItem(K key, V value, long tickCount) + : base(key, value) + { + this.TickCount = tickCount; + } + + /// + /// Gets or sets the tick count. + /// + public long TickCount { get; set; } + } +} diff --git a/BitFaster.Caching/Lru/LruItem.cs b/BitFaster.Caching/Lru/LruItem.cs index 56944316..2e8d0da5 100644 --- a/BitFaster.Caching/Lru/LruItem.cs +++ b/BitFaster.Caching/Lru/LruItem.cs @@ -1,53 +1,92 @@ - -namespace BitFaster.Caching.Lru -{ - /// - /// Represents an LRU item. - /// - /// The type of the key. - /// The type of the value. - public class LruItem - { - private volatile bool wasAccessed; - private volatile bool wasRemoved; - - /// - /// Initializes a new instance of the LruItem class with the specified key and value. - /// - /// The key. - /// The value. - public LruItem(K k, V v) - { - this.Key = k; - this.Value = v; - } - - /// - /// Gets the key. - /// - public readonly K Key; - - /// - /// Gets or sets the value. - /// - public V Value { get; set; } - - /// - /// Gets or sets a value indicating whether the item was accessed. - /// - public bool WasAccessed - { - get => this.wasAccessed; - set => this.wasAccessed = value; - } - - /// - /// Gets or sets a value indicating whether the item was removed. - /// - public bool WasRemoved - { - get => this.wasRemoved; - set => this.wasRemoved = value; - } - } -} + +using System; +using System.Collections.Generic; +using System.Runtime.CompilerServices; + +namespace BitFaster.Caching.Lru +{ + /// + /// Represents an LRU item. + /// + /// The type of the key. + /// The type of the value. + public class LruItem : IEquatable?> + { + private volatile bool wasAccessed; + private volatile bool wasRemoved; + + /// + /// Initializes a new instance of the LruItem class with the specified key and value. + /// + /// The key. + /// The value. + public LruItem(K k, V v) + { + this.Key = k; + this.Value = v; + } + + /// + /// Gets the key. + /// + public readonly K Key; + + /// + /// Gets or sets the value. + /// + public V Value { get; set; } + + /// + /// Gets or sets a value indicating whether the item was accessed. + /// + public bool WasAccessed + { + get => this.wasAccessed; + set => this.wasAccessed = value; + } + + /// + /// Gets or sets a value indicating whether the item was removed. + /// + public bool WasRemoved + { + get => this.wasRemoved; + set => this.wasRemoved = value; + } + + /// + public override bool Equals(object? obj) + { + return Equals(obj as LruItem); + } + + /// + public bool Equals(LruItem? other) + { + return ReferenceEquals(this, other); + } + + /// + public override int GetHashCode() + { + return Hash(Key, Value); + } + + /// + /// Compute the hash code for the specified key and value. + /// + /// The key + /// The value + /// The hash code + [MethodImpl(MethodImplOptions.AggressiveInlining)] + protected static int Hash(K key, V value) + { + unchecked + { + int hash = 486187739 ^ key?.GetHashCode() ?? (int)2166136261; + hash = (hash * 16777619) ^ value?.GetHashCode() ?? 486187739; + return hash; + } + } + } +} diff --git a/BitFaster.Caching/TypeProps.cs b/BitFaster.Caching/TypeProps.cs new file mode 100644 index 00000000..00c23d31 --- /dev/null +++ b/BitFaster.Caching/TypeProps.cs @@ -0,0 +1,46 @@ +using System; + +namespace BitFaster.Caching +{ + // https://source.dot.net/#System.Collections.Concurrent/System/Collections/Concurrent/ConcurrentDictionary.cs,2293 + internal static class TypeProps + { + /// Whether T's type can be written atomically (i.e., with no danger of torn reads). + internal static readonly bool IsWriteAtomic = IsWriteAtomicPrivate(); + + private static bool IsWriteAtomicPrivate() + { + // Section 12.6.6 of ECMA CLI explains which types can be read and written atomically without + // the risk of tearing. See https://www.ecma-international.org/publications/files/ECMA-ST/ECMA-335.pdf + + if (!typeof(T).IsValueType || + typeof(T) == typeof(IntPtr) || + typeof(T) == typeof(UIntPtr)) + { + return true; + } + + switch (Type.GetTypeCode(typeof(T))) + { + case TypeCode.Boolean: + case TypeCode.Byte: + case TypeCode.Char: + case TypeCode.Int16: + case TypeCode.Int32: + case TypeCode.SByte: + case TypeCode.Single: + case TypeCode.UInt16: + case TypeCode.UInt32: + return true; + + case TypeCode.Double: + case TypeCode.Int64: + case TypeCode.UInt64: + return IntPtr.Size == 8; + + default: + return false; + } + } + } +}