LFU after access fails to update expiry when read buffer is not full (#603)

* quick fix

* proper test

* update time on read

* rem delayable

---------

Author: bitfaster

BitFaster.Caching.UnitTests/Lfu/ConcurrentTLfuTests.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Runtime.InteropServices;
+using System.Threading;
 using BitFaster.Caching.Lfu;
 using BitFaster.Caching.Scheduler;
 using BitFaster.Caching.UnitTests.Retry;
@@ -25,6 +26,36 @@ public ConcurrentTLfuTests()
             lfu = new ConcurrentTLfu<int, string>(capacity, new ExpireAfterWrite<int, string>(timeToLive));
         }
 
+        // This is a scenario test to verify maintenance is run promptly after read.
+        [RetryFact]
+        public void WhenItemIsAccessedTimeToExpireIsUpdated()
+        { 
+            var cache = new ConcurrentLfuBuilder<int, int>()
+                .WithCapacity(10)
+                .WithExpireAfterAccess(TimeSpan.FromSeconds(5))
+                .Build();
+
+            Timed.Execute(
+                cache,
+                cache =>
+                {
+                    cache.AddOrUpdate(1, 1);
+                    return cache;
+                },
+                TimeSpan.FromSeconds(4),
+                cache =>
+                {
+                    cache.TryGet(1, out var value);
+                },
+                TimeSpan.FromSeconds(2),
+                cache =>
+                { 
+                    cache.TryGet(1, out var value).Should().BeTrue();
+                    cache.TryGet(1, out value).Should().BeTrue();
+                }
+            );
+        }
+
         [Fact]
         public void ConstructAddAndRetrieveWithCustomComparerReturnsValue()
         {

BitFaster.Caching/Lfu/ConcurrentLfuCore.cs

@@ -276,6 +276,7 @@ private bool TryGetImpl(K key, [MaybeNullWhen(false)] out V value)
                     {
                         TryScheduleDrain();
                     }
+                    this.policy.OnRead(node);
                     value = node.Value;
                     return true;
                 }
@@ -366,6 +367,7 @@ public bool TryUpdate(K key, V value)
                 // and we will just lose ordering/hit count, but not orphan the node.
                 this.writeBuffer.TryAdd(node);
                 TryScheduleDrain();
+                this.policy.OnWrite(node);
                 return true;
             }
 
@@ -525,8 +527,6 @@ private bool Maintenance(N? droppedWrite = null)
         {
             this.drainStatus.VolatileWrite(DrainStatus.ProcessingToIdle);
 
-            policy.AdvanceTime();
-
             // Note: this is only Span on .NET Core 3.1+, else this is no-op and it is still an array
             var buffer = this.drainBuffer.AsSpanOrArray();
 
@@ -601,7 +601,7 @@ private void OnAccess(N node)
                     break;
             }
 
-            policy.OnRead(node);
+            policy.AfterRead(node);
         }
 
         private void OnWrite(N node)
@@ -650,7 +650,7 @@ private void OnWrite(N node)
                     break;
             }
 
-            policy.OnWrite(node);
+            policy.AfterWrite(node);
         }
 
         private void PromoteProbation(LfuNode<K, V> node)

BitFaster.Caching/Lfu/NodePolicy.cs

@@ -10,9 +10,10 @@ internal interface INodePolicy<K, V, N>
     {
         N Create(K key, V value);
         bool IsExpired(N node);
-        void AdvanceTime();
         void OnRead(N node);
         void OnWrite(N node);
+        void AfterRead(N node);
+        void AfterWrite(N node);
         void OnEvict(N node);
         void ExpireEntries<P>(ref ConcurrentLfuCore<K, V, N, P> cache) where P : struct, INodePolicy<K, V, N>;
     }
@@ -33,17 +34,22 @@ public bool IsExpired(AccessOrderNode<K, V> node)
         }
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public void AdvanceTime()
+        public void OnRead(AccessOrderNode<K, V> node)
         {
         }
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public void OnRead(AccessOrderNode<K, V> node)
+        public void OnWrite(AccessOrderNode<K, V> node)
         {
         }
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public void OnWrite(AccessOrderNode<K, V> node)
+        public void AfterRead(AccessOrderNode<K, V> node)
+        {
+        }
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public void AfterWrite(AccessOrderNode<K, V> node)
         {
         }
 
@@ -84,29 +90,33 @@ public TimeOrderNode<K, V> Create(K key, V value)
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public bool IsExpired(TimeOrderNode<K, V> node)
-        {
-            return node.TimeToExpire < Duration.SinceEpoch();
-        }
-
-        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public void AdvanceTime()
         {
             current = Duration.SinceEpoch();
+            return node.TimeToExpire < current;
         }
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public void OnRead(TimeOrderNode<K, V> node)
         {
-            var oldTte = node.TimeToExpire;
+            // we know IsExpired is always called immediate before OnRead, so piggyback on the current time
             node.TimeToExpire = current + expiryCalculator.GetExpireAfterRead(node.Key, node.Value, node.TimeToExpire - current);
-            if (oldTte.raw != node.TimeToExpire.raw)
-            {
-                wheel.Reschedule(node);
-            }
         }
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public void OnWrite(TimeOrderNode<K, V> node)
+        {
+            var current = Duration.SinceEpoch();
+            node.TimeToExpire = current + expiryCalculator.GetExpireAfterUpdate(node.Key, node.Value, node.TimeToExpire - current);
+        }
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public void AfterRead(TimeOrderNode<K, V> node)
+        {
+            wheel.Reschedule(node);
+        }
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public void AfterWrite(TimeOrderNode<K, V> node)
         {
             // if the node is not yet scheduled, it is being created
             // the time is set on create in case it is read before the buffer is processed
@@ -116,12 +126,7 @@ public void OnWrite(TimeOrderNode<K, V> node)
             }
             else
             {
-                var oldTte = node.TimeToExpire;
-                node.TimeToExpire = current + expiryCalculator.GetExpireAfterUpdate(node.Key, node.Value, node.TimeToExpire - current);
-                if (oldTte.raw != node.TimeToExpire.raw)
-                {
-                    wheel.Reschedule(node);
-                }
+                wheel.Reschedule(node);
             }
         }
 
@@ -134,7 +139,7 @@ public void OnEvict(TimeOrderNode<K, V> node)
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public void ExpireEntries<P>(ref ConcurrentLfuCore<K, V, TimeOrderNode<K, V>, P> cache) where P : struct, INodePolicy<K, V, TimeOrderNode<K, V>>
         {
-            wheel.Advance(ref cache, current);
+            wheel.Advance(ref cache, Duration.SinceEpoch());
         }
     }
 }