Fix - refactor trim against unhandled exceptions, high write traffic …

…and make it more robust to concurrent changes (#90)

src/FastCache.Cached/CacheManager.cs

@@ -96,7 +96,6 @@ public static IEnumerable<Cached<K, V>> EnumerateEntries<K, V>() where K : notnu
     /// </summary>
     /// <typeparam name="K">Cache entry key type. string, int or (int, int) for multi-key.</typeparam>
     /// <typeparam name="V">Cache entry value type</typeparam>
-    /// <param name="percentage"></param>
     /// <returns>True: trim is performed inline. False: the count to trim is above threshold and removal is queued to thread pool.</returns>
     public static bool Trim<K, V>(double percentage) where K : notnull
     {
@@ -105,52 +104,63 @@ public static bool Trim<K, V>(double percentage) where K : notnull
             ThrowHelpers.ArgumentOutOfRange(percentage, nameof(percentage));
         }
 
-        if (CacheStaticHolder<K, V>.QuickList.InProgress)
+        var store = CacheStaticHolder<K, V>.Store;
+        var count = store.Count;
+        var target = (uint)count - (uint)(count * (percentage / 100.0));
+        var toTrim = count - target;
+
+        var trimmed = Trim<K, V>(target, percentage);
+        if (trimmed >= toTrim)
         {
-            // Bail out early if the items are being removed via quick list.
-            return false;
+            return true;
         }
+        toTrim -= trimmed;
 
-        static void ExecuteTrim(uint trimCount, bool takeLock)
+        ThreadPool.QueueUserWorkItem(target =>
         {
-            var removedFromQuickList = CacheStaticHolder<K, V>.QuickList.Trim(trimCount);
-            if (removedFromQuickList >= trimCount)
-            {
-                return;
-            }
-
-            if (takeLock && !CacheStaticHolder<K, V>.QuickList.TryLock())
+            var store = CacheStaticHolder<K, V>.Store;
+            foreach (var (key, _) in store)
             {
-                return;
+                if (target-- <= 0) break;
+                store.TryRemove(key, out _);
             }
+        }, toTrim, preferLocal: false);
 
-            var removed = 0;
-            var store = CacheStaticHolder<K, V>.Store;
-            var enumerator = store.GetEnumerator();
-            var toRemoveFromStore = trimCount - removedFromQuickList;
+        return false;
+    }
 
-            while (removed < toRemoveFromStore && enumerator.MoveNext())
-            {
-                store.TryRemove(enumerator.Current.Key, out _);
-                removed++;
-            }
+    internal static uint Trim<K, V>(uint target, double percentage) where K : notnull
+    {
+        var store = CacheStaticHolder<K, V>.Store;
+        var quickList = CacheStaticHolder<K, V>.QuickList;
 
-            if (takeLock)
-            {
-                CacheStaticHolder<K, V>.QuickList.Release();
-            }
+        if (quickList.InProgress && store.Count < target)
+        {
+            // Bail out early if a concurrent trim is already in progress and the count is below the limit.
+            return target;
         }
 
-        var trimCount = Math.Max(1, (uint)(CacheStaticHolder<K, V>.Store.Count * (percentage / 100.0)));
-        if (trimCount <= Constants.InlineTrimCountThreshold)
+        var trimCount = Math.Min(
+            Math.Max(1, (uint)(store.Count * (percentage / 100.0))),
+            Constants.InlineTrimCountLimit);
+
+        var removedFromQuickList = quickList.Trim(trimCount);
+        if (removedFromQuickList >= trimCount || store.Count <= target)
         {
-            ExecuteTrim(trimCount, takeLock: false);
-            return true;
+            return removedFromQuickList;
         }
 
-        ThreadPool.QueueUserWorkItem(static count => ExecuteTrim(count, takeLock: true), trimCount, preferLocal: true);
+        var removed = 0;
+        var enumerator = store.GetEnumerator();
+        var toRemoveFromStore = trimCount - removedFromQuickList;
 
-        return false;
+        while (removed < toRemoveFromStore && enumerator.MoveNext())
+        {
+            store.TryRemove(enumerator.Current.Key, out _);
+            removed++;
+        }
+
+        return (uint)removed + removedFromQuickList;
     }
 
     /// <summary>

src/FastCache.Cached/Cached.cs

@@ -61,19 +61,21 @@ public V Save(V value, TimeSpan expiration)
 
     /// <summary>
     /// Saves the value to cache. The value will expire once the provided interval of time passes.
-    /// If the cache entries count is equal or above the limit, the cache is either trimmed to make place
-    /// or trimming is queued on a threadpool and the value is returned as is.
+    /// If the cache entries count is equal or above the limit, the cache is trimmed to make place
+    /// for the new entry.
     /// </summary>
     /// <param name="value">Value to save</param>
     /// <param name="expiration">Interval in which the value will expire</param>
     /// <param name="limit">Limit for cache entries count</param>
     /// <returns>Saved value</returns>
     public V Save(V value, TimeSpan expiration, uint limit)
     {
-        return CacheStaticHolder<K, V>.Store.Count < limit
-            || CacheManager.Trim<K, V>(Constants.FullCapacityTrimPercentage)
-                ? Save(value, expiration)
-                : value;
+        if (CacheStaticHolder<K, V>.Store.Count >= limit)
+        {
+            CacheManager.Trim<K, V>(limit, Constants.FullCapacityTrimPercentage);
+        }
+
+        return Save(value, expiration);
     }
 
     /// <summary>
@@ -107,18 +109,12 @@ public void Remove()
 }
 
 [StructLayout(LayoutKind.Auto)]
-internal readonly struct CachedInner<T>
+[method: MethodImpl(MethodImplOptions.AggressiveInlining)]
+internal readonly struct CachedInner<T>(T value, long timestamp)
 {
-    internal readonly long _timestamp;
-
-    public readonly T Value;
+    internal readonly long _timestamp = timestamp;
 
-    [MethodImpl(MethodImplOptions.AggressiveInlining)]
-    public CachedInner(T value, long timestamp)
-    {
-        Value = value;
-        _timestamp = timestamp;
-    }
+    public readonly T Value = value;
 
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
     public bool IsNotExpired() => TimeUtils.Now < _timestamp;

src/FastCache.Cached/Constants.cs

@@ -43,11 +43,13 @@ internal static class Constants
     public static readonly uint ParallelSaveMinBatchSize = uint
         .TryParse(GetVar("FASTCACHE_PARALLEL_SAVE_MIN_BATCH_SIZE"), out var parsed) ? parsed : DefaultParallelSaveMinBatchSize;
 
-    public static readonly double FullCapacityTrimPercentage = double
-        .TryParse(GetVar("FASTCACHE_FULL_CAPACITY_TRIM_PERCENT"), out var parsed) ? parsed : QuickListAdjustableLengthPercentage;
+    public static readonly double FullCapacityTrimPercentage = Math.Clamp(
+        double.TryParse(
+            GetVar("FASTCACHE_FULL_CAPACITY_TRIM_PERCENT"), out var parsed) ? parsed : QuickListAdjustableLengthPercentage,
+        0.1, 99.9);
 
-    public static readonly uint InlineTrimCountThreshold = uint
-        .TryParse(GetVar("FASTCACHE_INLINE_TRIM_COUNT_THRESHOLD"), out var parsed) ? parsed : 256;
+    public static readonly uint InlineTrimCountLimit = uint
+        .TryParse(GetVar("FASTCACHE_INLINE_TRIM_COUNT_LIMIT"), out var parsed) ? parsed : 512;
 
     public static readonly bool ConsiderFullGC = bool.TryParse(GetVar("FASTCACHE_CONSIDER_GC"), out var parsed) && parsed;
 

src/FastCache.Cached/EvictionJob.cs

@@ -1,5 +1,6 @@
 using FastCache.Services;
 using FastCache.Helpers;
+using System.Diagnostics;
 
 namespace FastCache;
 
@@ -36,11 +37,9 @@ public EvictionJob()
                 {
                     CacheStaticHolder<K, V>.QuickList.Evict();
                 }
-                catch
+                catch (Exception ex)
                 {
-#if DEBUG
-                    throw;
-#endif
+                    Debug.Fail(ex.Message);
                 }
             },
             null,

src/FastCache.Cached/EvictionQuickList.cs

@@ -32,7 +32,7 @@ public EvictionQuickList()
     public void Add(K key, long expiresAt)
     {
         var entries = _active;
-        var count = (uint)_count;
+        var count = (uint)(ulong)_count;
         if (count < entries.Length)
         {
             entries[count] = (key, expiresAt);
@@ -53,7 +53,7 @@ public void Add(K key, long expiresAt)
     internal void OverwritingAdd(K key, long expiresAt)
     {
         var entries = _active;
-        var count = (uint)_count;
+        var count = (uint)(ulong)_count;
         if (count < entries.Length)
         {
             entries[count] = (key, expiresAt);
@@ -242,24 +242,31 @@ internal uint Trim(uint count)
             return 0;
         }
 
+        var active = _active;
         uint currentCount = AtomicCount;
-        uint toRemoveCount = Math.Min(currentCount, count);
+        uint toTrim = Math.Min(currentCount, count);
 
-        uint countAfterTrim = currentCount - toRemoveCount;
+        var trimEntries = active.AsSpan(
+            (int)(currentCount - toTrim), (int)toTrim);
 
+        if (trimEntries.IsEmpty)
+        {
+            return 0;
+        }
+
+        var removed = 0;
         var store = CacheStaticHolder<K, V>.Store;
-        uint removed = 0;
-        for (uint i = countAfterTrim; i < countAfterTrim + toRemoveCount; i++)
+        foreach (var (key, _) in trimEntries)
         {
-            if (store.TryRemove(_active[i].Key, out var _))
+            if (store.TryRemove(key, out _))
             {
                 removed++;
             }
         }
 
-        Interlocked.Exchange(ref _count, countAfterTrim);
+        Interlocked.Exchange(ref _count, currentCount - toTrim);
         _evictionLock.Release();
-        return removed;
+        return (uint)removed;
     }
 
     internal void PullFromCacheStore()

tests/FastCache.CachedTests/Cached_TryGetAndSave.cs

@@ -39,7 +39,7 @@ public void Cached_Saved_ReturnsTrueAndCorrectValue()
     [Fact]
     public void Cached_SaveWithLimit_TrimsOnMaxCapacity_And_ReplacesValue()
     {
-        var limit = (int)(Constants.InlineTrimCountThreshold * (100.0 / Constants.FullCapacityTrimPercentage));
+        var limit = (int)(Constants.InlineTrimCountLimit * (100.0 / Constants.FullCapacityTrimPercentage));
 
         for (uint i = 0; i < limit; i++)
         {
@@ -63,34 +63,6 @@ public void Cached_SaveWithLimit_TrimsOnMaxCapacity_And_ReplacesValue()
         Assert.True(CacheStaticHolder<string, InlineTrimItem>.Store.Count < limit);
     }
 
-    [Fact]
-    public async Task Cached_SaveWithLimit_DoesNotSaveOnOverCapacity_And_OverInlineTrimThreshold()
-    {
-        var limit = (int)(Constants.QuickListMinLength * (100.0 / Constants.FullCapacityTrimPercentage)) + Constants.InlineTrimCountThreshold;
-
-        for (uint i = 0; i < limit; i++)
-        {
-            var item = new NonInlineTrimItem(GetRandomString());
-
-            item.Cache(GetTestKey(i), TimeSpan.MaxValue);
-        }
-
-        Assert.Equal(limit, CacheStaticHolder<string, NonInlineTrimItem>.Store.Count);
-
-        var key = GetTestKey(-1);
-        var foundBefore = Cached<NonInlineTrimItem>.TryGet(key, out var cached);
-        cached.Save(new NonInlineTrimItem(GetRandomString()), TimeSpan.MaxValue, (uint)limit);
-
-        Assert.False(foundBefore);
-
-        var foundAfter = Cached<NonInlineTrimItem>.TryGet(key, out _);
-
-        Assert.False(foundAfter);
-
-        await Task.Delay(500);
-        Assert.True(CacheStaticHolder<string, NonInlineTrimItem>.Store.Count < limit);
-    }
-
     [Fact]
     public void Cached_Remove_Removes()
     {