diff --git a/BitFaster.Caching.UnitTests/Lfu/ConcurrentLfuSoakTests.cs b/BitFaster.Caching.UnitTests/Lfu/ConcurrentLfuSoakTests.cs index 67a0cec0..9c9f29f9 100644 --- a/BitFaster.Caching.UnitTests/Lfu/ConcurrentLfuSoakTests.cs +++ b/BitFaster.Caching.UnitTests/Lfu/ConcurrentLfuSoakTests.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Diagnostics; using System.Reflection; +using System.Threading; using System.Threading.Tasks; using BitFaster.Caching.Buffers; using BitFaster.Caching.Lfu; @@ -316,6 +317,50 @@ public async Task WhenConcurrentUpdateAndRemoveKvp() await removal; } + // This test will run forever if there is a live lock. + // Since the cache bookkeeping has some overhead, it is harder to provoke + // spinning inside the reader thread compared to LruItemSoakTests.DetectTornStruct. + [Theory] + [Repeat(10)] + public async Task WhenValueIsBigStructNoLiveLock(int _) + { + using var source = new CancellationTokenSource(); + var started = new TaskCompletionSource(); + var cache = new ConcurrentLfu(1, 20, new BackgroundThreadScheduler(), EqualityComparer.Default); + + var setTask = Task.Run(() => Setter(cache, source.Token, started)); + await started.Task; + Checker(cache, source); + + await setTask; + } + + private void Setter(ICache cache, CancellationToken cancelToken, TaskCompletionSource started) + { + started.SetResult(true); + + while (true) + { + cache.AddOrUpdate(1, Guid.NewGuid()); + cache.AddOrUpdate(1, Guid.NewGuid()); + + if (cancelToken.IsCancellationRequested) + { + return; + } + } + } + + private void Checker(ICache cache,CancellationTokenSource source) + { + for (int count = 0; count < 100_000; ++count) + { + cache.TryGet(1, out _); + } + + source.Cancel(); + } + private ConcurrentLfu CreateWithBackgroundScheduler() { var scheduler = new BackgroundThreadScheduler(); diff --git a/BitFaster.Caching.UnitTests/Lfu/ConcurrentLfuTests.cs b/BitFaster.Caching.UnitTests/Lfu/ConcurrentLfuTests.cs index b806d867..0f3756bc 100644 --- a/BitFaster.Caching.UnitTests/Lfu/ConcurrentLfuTests.cs +++ b/BitFaster.Caching.UnitTests/Lfu/ConcurrentLfuTests.cs @@ -600,6 +600,19 @@ public void WhenItemIsUpdatedItIsUpdated() value.Should().Be(2); } + [Fact] + public void WhenKeyExistsAddOrUpdateGuidUpdatesExistingItem() + { + var lfu2 = new ConcurrentLfu(1, 40, new BackgroundThreadScheduler(), EqualityComparer.Default); + + var b = new byte[8]; + lfu2.AddOrUpdate(1, new Guid(1, 0, 0, b)); + lfu2.AddOrUpdate(1, new Guid(2, 0, 0, b)); + + lfu2.TryGet(1, out var value).Should().BeTrue(); + value.Should().Be(new Guid(2, 0, 0, b)); + } + [Fact] public void WhenItemDoesNotExistUpdatedAddsItem() { diff --git a/BitFaster.Caching.UnitTests/Lfu/LfuNodeSoakTest.cs b/BitFaster.Caching.UnitTests/Lfu/LfuNodeSoakTest.cs new file mode 100644 index 00000000..9fa69f1d --- /dev/null +++ b/BitFaster.Caching.UnitTests/Lfu/LfuNodeSoakTest.cs @@ -0,0 +1,65 @@ +using System; +using System.Threading; +using System.Threading.Tasks; +using BitFaster.Caching.Lfu; +using Xunit; +using static BitFaster.Caching.UnitTests.Lru.LruItemSoakTests; + +namespace BitFaster.Caching.UnitTests.Lfu +{ + [Collection("Soak")] + public class LfuNodeSoakTest + { + private const int soakIterations = 3; + private readonly LfuNode item = new(1, MassiveStruct.A); + + // Adapted from + // https://stackoverflow.com/questions/23262513/reproduce-torn-reads-of-decimal-in-c-sharp + [Theory] + [Repeat(soakIterations)] + public async Task DetectTornStruct(int _) + { + using var source = new CancellationTokenSource(); + var started = new TaskCompletionSource(); + + var setTask = Task.Run(() => Setter(source.Token, started)); + await started.Task; + Checker(source); + + await setTask; + } + + private void Setter(CancellationToken cancelToken, TaskCompletionSource started) + { + started.SetResult(true); + + while (true) + { + item.SeqLockWrite(MassiveStruct.A); + item.SeqLockWrite(MassiveStruct.B); + + if (cancelToken.IsCancellationRequested) + { + return; + } + } + } + + private void Checker(CancellationTokenSource source) + { + // On my machine, without SeqLock, this consistently fails below 100 iterations + // on debug build, and below 1000 on release build + for (int count = 0; count < 10_000; ++count) + { + var t = item.SeqLockRead(); + + if (t != MassiveStruct.A && t != MassiveStruct.B) + { + throw new Exception($"Value is torn after {count} iterations"); + } + } + + source.Cancel(); + } + } +} diff --git a/BitFaster.Caching.UnitTests/Lfu/NodeMemoryLayoutDumps.cs b/BitFaster.Caching.UnitTests/Lfu/NodeMemoryLayoutDumps.cs index 3ed11a65..f0b6cd04 100644 --- a/BitFaster.Caching.UnitTests/Lfu/NodeMemoryLayoutDumps.cs +++ b/BitFaster.Caching.UnitTests/Lfu/NodeMemoryLayoutDumps.cs @@ -15,33 +15,33 @@ public NodeMemoryLayoutDumps(ITestOutputHelper testOutputHelper) } //Type layout for 'AccessOrderNode`2' - //Size: 48 bytes.Paddings: 2 bytes(%4 of empty space) - //|====================================================| - //| Object Header(8 bytes) | - //|----------------------------------------------------| - //| Method Table Ptr(8 bytes) | - //|====================================================| - //| 0-7: LfuNodeList`2 list(8 bytes) | - //|----------------------------------------------------| - //| 8-15: LfuNode`2 next(8 bytes) | - //|----------------------------------------------------| - //| 16-23: LfuNode`2 prev(8 bytes) | - //|----------------------------------------------------| - //| 24-31: Object Key(8 bytes) | - //|----------------------------------------------------| - //| 32-39: Object k__BackingField(8 bytes) | - //|----------------------------------------------------| - //| 40-43: Position k__BackingField(4 bytes) | - //| |===============================| | - //| | 0-3: Int32 value__(4 bytes) | | - //| |===============================| | - //|----------------------------------------------------| - //| 44: Boolean wasRemoved(1 byte) | - //|----------------------------------------------------| - //| 45: Boolean wasDeleted(1 byte) | - //|----------------------------------------------------| - //| 46-47: padding(2 bytes) | - //|====================================================| + //Size: 48 bytes. Paddings: 0 bytes (%0 of empty space) + //|=====================================================| + //| Object Header (8 bytes) | + //|-----------------------------------------------------| + //| Method Table Ptr (8 bytes) | + //|=====================================================| + //| 0-7: Object data (8 bytes) | + //|-----------------------------------------------------| + //| 8-15: LfuNodeList`2 list (8 bytes) | + //|-----------------------------------------------------| + //| 16-23: LfuNode`2 next (8 bytes) | + //|-----------------------------------------------------| + //| 24-31: LfuNode`2 prev (8 bytes) | + //|-----------------------------------------------------| + //| 32-39: Object Key (8 bytes) | + //|-----------------------------------------------------| + //| 40-43: Int32 sequence (4 bytes) | + //|-----------------------------------------------------| + //| 44-45: Position k__BackingField (2 bytes) | + //| |================================| | + //| | 0-1: Int16 value__ (2 bytes) | | + //| |================================| | + //|-----------------------------------------------------| + //| 46: Boolean wasRemoved (1 byte) | + //|-----------------------------------------------------| + //| 47: Boolean wasDeleted (1 byte) | + //|=====================================================| [Fact] public void DumpAccessOrderNode() { @@ -49,43 +49,43 @@ public void DumpAccessOrderNode() testOutputHelper.WriteLine(layout.ToString()); } - //Type layout for 'TimeOrderNode`2' - //Size: 72 bytes.Paddings: 2 bytes(%2 of empty space) - //|====================================================| - //| Object Header(8 bytes) | - //|----------------------------------------------------| - //| Method Table Ptr(8 bytes) | - //|====================================================| - //| 0-7: LfuNodeList`2 list(8 bytes) | - //|----------------------------------------------------| - //| 8-15: LfuNode`2 next(8 bytes) | - //|----------------------------------------------------| - //| 16-23: LfuNode`2 prev(8 bytes) | - //|----------------------------------------------------| - //| 24-31: Object Key(8 bytes) | - //|----------------------------------------------------| - //| 32-39: Object k__BackingField(8 bytes) | - //|----------------------------------------------------| - //| 40-43: Position k__BackingField(4 bytes) | - //| |===============================| | - //| | 0-3: Int32 value__(4 bytes) | | - //| |===============================| | - //|----------------------------------------------------| - //| 44: Boolean wasRemoved(1 byte) | - //|----------------------------------------------------| - //| 45: Boolean wasDeleted(1 byte) | - //|----------------------------------------------------| - //| 46-47: padding(2 bytes) | - //|----------------------------------------------------| - //| 48-55: TimeOrderNode`2 prevTime(8 bytes) | - //|----------------------------------------------------| - //| 56-63: TimeOrderNode`2 nextTime(8 bytes) | - //|----------------------------------------------------| - //| 64-71: Duration timeToExpire(8 bytes) | - //| |===========================| | - //| | 0-7: Int64 raw(8 bytes) | | - //| |===========================| | - //|====================================================| + // Type layout for 'TimeOrderNode`2' + //Size: 72 bytes. Paddings: 0 bytes (%0 of empty space) + //|=====================================================| + //| Object Header (8 bytes) | + //|-----------------------------------------------------| + //| Method Table Ptr (8 bytes) | + //|=====================================================| + //| 0-7: Object data (8 bytes) | + //|-----------------------------------------------------| + //| 8-15: LfuNodeList`2 list (8 bytes) | + //|-----------------------------------------------------| + //| 16-23: LfuNode`2 next (8 bytes) | + //|-----------------------------------------------------| + //| 24-31: LfuNode`2 prev (8 bytes) | + //|-----------------------------------------------------| + //| 32-39: Object Key (8 bytes) | + //|-----------------------------------------------------| + //| 40-43: Int32 sequence (4 bytes) | + //|-----------------------------------------------------| + //| 44-45: Position k__BackingField (2 bytes) | + //| |================================| | + //| | 0-1: Int16 value__ (2 bytes) | | + //| |================================| | + //|-----------------------------------------------------| + //| 46: Boolean wasRemoved (1 byte) | + //|-----------------------------------------------------| + //| 47: Boolean wasDeleted (1 byte) | + //|-----------------------------------------------------| + //| 48-55: TimeOrderNode`2 prevTime (8 bytes) | + //|-----------------------------------------------------| + //| 56-63: TimeOrderNode`2 nextTime (8 bytes) | + //|-----------------------------------------------------| + //| 64-71: Duration timeToExpire (8 bytes) | + //| |============================| | + //| | 0-7: Int64 raw (8 bytes) | | + //| |============================| | + //|=====================================================| [Fact] public void DumpTimeOrderNode() { diff --git a/BitFaster.Caching.UnitTests/Lru/ConcurrentLruSoakTests.cs b/BitFaster.Caching.UnitTests/Lru/ConcurrentLruSoakTests.cs index ff90bba6..def2f841 100644 --- a/BitFaster.Caching.UnitTests/Lru/ConcurrentLruSoakTests.cs +++ b/BitFaster.Caching.UnitTests/Lru/ConcurrentLruSoakTests.cs @@ -345,8 +345,6 @@ private void Setter(ICache cache, CancellationToken cancelToken, Task private void Checker(ICache cache,CancellationTokenSource source) { - // On my machine, without SeqLock, this consistently fails below 100 iterations - // on debug build, and below 1000 on release build for (int count = 0; count < 100_000; ++count) { cache.TryGet(1, out _); diff --git a/BitFaster.Caching/Lfu/LfuNode.cs b/BitFaster.Caching/Lfu/LfuNode.cs index 5d37ad5e..de5b7ce7 100644 --- a/BitFaster.Caching/Lfu/LfuNode.cs +++ b/BitFaster.Caching/Lfu/LfuNode.cs @@ -1,9 +1,14 @@ #nullable disable +using System.Runtime.CompilerServices; +using System.Threading; + namespace BitFaster.Caching.Lfu { internal class LfuNode where K : notnull { + private V data; + internal LfuNodeList list; internal LfuNode next; internal LfuNode prev; @@ -11,15 +16,44 @@ internal class LfuNode private volatile bool wasRemoved; private volatile bool wasDeleted; + // only used when V is a non-atomic value type to prevent torn reads + private int sequence; + public LfuNode(K k, V v) { this.Key = k; - this.Value = v; + this.data = v; } public readonly K Key; - public V Value { get; set; } + public V Value + { + [MethodImpl(MethodImplOptions.AggressiveInlining)] + get + { + if (TypeProps.IsWriteAtomic) + { + return data; + } + else + { + return SeqLockRead(); + } + } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + set + { + if (TypeProps.IsWriteAtomic) + { + data = value; + } + else + { + SeqLockWrite(value); + } + } + } public Position Position { get; set; } @@ -57,9 +91,43 @@ internal void Invalidate() next = null; prev = null; } + + internal V SeqLockRead() + { + var spin = new SpinWait(); + while (true) + { + var start = Volatile.Read(ref this.sequence); + + if ((start & 1) == 1) + { + // A write is in progress, spin. + spin.SpinOnce(); + continue; + } + + V copy = this.data; + + var end = Volatile.Read(ref this.sequence); + if (start == end) + { + return copy; + } + } + } + + // Note: LfuNode should be locked while invoking this method. Multiple writer threads are not supported. + internal void SeqLockWrite(V value) + { + Interlocked.Increment(ref sequence); + + this.data = value; + + Interlocked.Increment(ref sequence); + } } - internal enum Position + internal enum Position : short { Window, Probation,