Mitigate value type torn writes during LRU update (enqueue)

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<int, Guid>(1, capacity, EqualityComparer<int>.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<int, Guid, LruItem<int, Guid>, LruPolicy<int, Guid>, TelemetryPolicy<int, Guid>>(lruVT).Validate();
+            }
+        }
+
         [Fact]
         public async Task WhenSoakConcurrentGetAndAddCacheEndsInConsistentState()
         {

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<int, Guid>(1, capacity, EqualityComparer<int>.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<int, Guid, LruItem<int, Guid>, LruPolicy<int, Guid>, TelemetryPolicy<int, Guid>>(lru2).Validate();
+        }
+
         [Fact]
         public void WhenKeyExistsTryUpdateDisposesOldValue()
         {
@@ -1357,7 +1373,7 @@ private void ValidateQueue(ConcurrentLruCore<K, V, I, P, T> 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");
                 }
             }
         }

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<int, int>(1, 2, 3);
+            var item2 = new LongTickCountLruItem<int, int>(2, 1, 0);
+
+            var d = new ConcurrentDictionary<int, LongTickCountLruItem<int, int>>();
+            d.TryAdd(1, item1);
+
+            d.TryUpdate(1, item2, item1).Should().BeTrue();
+        }
+    }
+}

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<int, int>(1, 2);
+
+            item.Equals(item).Should().BeTrue();
+        }
+
+        [Fact]
+        public void EqualsObjectWithSameReferenceReturnsTrue()
+        { 
+            var item = new LruItem<int, int>(1, 2);
+
+            item.Equals((object)item).Should().BeTrue();
+        }
+
+        [Fact]
+        public void EqualsWithSameValuesReturnsFalse()
+        { 
+            var item1 = new LruItem<int, int>(1, 2);
+            var item2 = new LruItem<int, int>(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<int, int>(1, 2);
+            var item2 = new LruItem<int, int>(2, 1);
+
+            item1.GetHashCode().Should().NotBe(item2.GetHashCode());
+        }
+
+        [Fact]
+        public void GetHashCodeHandlesNulls()
+        { 
+            var item1 = new LruItem<object, object>(null, null);
+            item1.GetHashCode().Should().NotBe(0);
+        }
+    }
+}

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<bool>().Which.Should().Be(expected);
+        }
+
+        private static bool IsWriteAtomic<T>()
+        { 
+            return TypeProps<T>.IsWriteAtomic;
+        }
+    }
+}

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<V>.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<V>.Dispose(oldValue);
 

BitFaster.Caching/Lru/LongTickCountLruItem.cs

@@ -1,28 +1,28 @@
-
-namespace BitFaster.Caching.Lru
-{
-    /// <summary>
-    /// Represents an LRU item that also stores tick count.
-    /// </summary>
-    /// <typeparam name="K">The type of the key.</typeparam>
-    /// <typeparam name="V">The type of the value.</typeparam>
-    public class LongTickCountLruItem<K, V> : LruItem<K, V>
-    {
-        /// <summary>
-        /// Initializes a new instance of the LongTickCountLruItem class with the specified key and value.
-        /// </summary>
-        /// <param name="key">The key.</param>
-        /// <param name="value">The value.</param>
-        /// <param name="tickCount">The tick count.</param>
-        public LongTickCountLruItem(K key, V value, long tickCount)
-            : base(key, value)
-        {
-            this.TickCount = tickCount;
-        }
-
-        /// <summary>
-        /// Gets or sets the tick count.
-        /// </summary>
-        public long TickCount { get; set; }
-    }
-}
+
+namespace BitFaster.Caching.Lru
+{
+    /// <summary>
+    /// Represents an LRU item that also stores tick count.
+    /// </summary>
+    /// <typeparam name="K">The type of the key.</typeparam>
+    /// <typeparam name="V">The type of the value.</typeparam>
+    public class LongTickCountLruItem<K, V> : LruItem<K, V>
+    {
+        /// <summary>
+        /// Initializes a new instance of the LongTickCountLruItem class with the specified key and value.
+        /// </summary>
+        /// <param name="key">The key.</param>
+        /// <param name="value">The value.</param>
+        /// <param name="tickCount">The tick count.</param>
+        public LongTickCountLruItem(K key, V value, long tickCount)
+            : base(key, value)
+        {
+            this.TickCount = tickCount;
+        }
+
+        /// <summary>
+        /// Gets or sets the tick count.
+        /// </summary>
+        public long TickCount { get; set; }
+    }
+}