zzzprojects#764 - ConstantExpressionHelper Singleton Factory; Code Review Resolutions

Author: Travis Whidden

src/System.Linq.Dynamic.Core/Parser/ConstantExpressionHelper.cs

@@ -1,59 +1,49 @@
 using System.Linq.Dynamic.Core.Util.Cache;
+using System.Linq.Dynamic.Core.Validation;
 using System.Linq.Expressions;
 
 namespace System.Linq.Dynamic.Core.Parser;
 
 internal class ConstantExpressionHelper
 {
-    private readonly ParsingConfig _config;
-
     // Static shared instance to prevent duplications of the same objects
-    private static ThreadSafeSlidingCache<object, Expression>? _expressions;
-    private static ThreadSafeSlidingCache<Expression, string>? _literals;
+    private readonly ThreadSafeSlidingCache<object, Expression> _expressions;
+    private readonly ThreadSafeSlidingCache<Expression, string> _literals;
 
     public ConstantExpressionHelper(ParsingConfig config)
     {
-        _config = config;
-        
-    }
+        var parsingConfig = Check.NotNull(config);
+        var cacheConfig = Check.NotNull(parsingConfig.ConstantExpressionCacheConfig);
 
-    private ThreadSafeSlidingCache<Expression, string> GetLiterals()
-    {
-        _literals ??= new ThreadSafeSlidingCache<Expression, string>(
-            _config.ConstantExpressionSlidingCacheTimeToLive,
-            _config.ConstantExpressionSlidingCacheCleanupFrequency,
-            _config.ConstantExpressionSlidingCacheMinItemsTrigger
+        _literals = new ThreadSafeSlidingCache<Expression, string>(
+            cacheConfig.TimeToLive,
+            cacheConfig.CleanupFrequency,
+            cacheConfig.MinItemsTrigger
         );
-        return _literals;
-    }
 
-    private ThreadSafeSlidingCache<object, Expression> GetExpression()
-    {
-        _expressions ??= new ThreadSafeSlidingCache<object, Expression>(
-            _config.ConstantExpressionSlidingCacheTimeToLive,
-            _config.ConstantExpressionSlidingCacheCleanupFrequency,
-            _config.ConstantExpressionSlidingCacheMinItemsTrigger
+        _expressions = new ThreadSafeSlidingCache<object, Expression>(
+            cacheConfig.TimeToLive,
+            cacheConfig.CleanupFrequency,
+            cacheConfig.MinItemsTrigger
         );
-        return _expressions;
     }
 
-
     public bool TryGetText(Expression expression, out string? text)
     {
-        return GetLiterals().TryGetValue(expression, out text);
+        return _literals.TryGetValue(expression, out text);
     }
 
     public Expression CreateLiteral(object value, string text)
     {
-        if (GetExpression().TryGetValue(value, out var outputValue))
+        if (_expressions.TryGetValue(value, out var outputValue))
         {
             return outputValue;
         }
 
         var constantExpression = Expression.Constant(value);
 
-        GetExpression().AddOrUpdate(value, constantExpression);
-        GetLiterals().AddOrUpdate(constantExpression, text);
+        _expressions.AddOrUpdate(value, constantExpression);
+        _literals.AddOrUpdate(constantExpression, text);
 
         return constantExpression;
     }

src/System.Linq.Dynamic.Core/Parser/ConstantExpressionHelperFactory.cs

@@ -0,0 +1,20 @@
+namespace System.Linq.Dynamic.Core.Parser;
+
+internal static class ConstantExpressionHelperFactory
+{
+    private static readonly object Lock = new();
+    private static ConstantExpressionHelper? _instance;
+
+    public static ConstantExpressionHelper GetInstance(ParsingConfig config)
+    {
+        if (_instance == null)
+        {
+            lock (Lock)
+            {
+                _instance ??= new ConstantExpressionHelper(config);
+            }
+        }
+
+        return _instance;
+    }
+}

src/System.Linq.Dynamic.Core/Parser/ExpressionParser.cs

@@ -82,7 +82,7 @@ public ExpressionParser(ParameterExpression[]? parameters, string expression, ob
         _methodFinder = new MethodFinder(_parsingConfig, _expressionHelper);
         _typeFinder = new TypeFinder(_parsingConfig, _keywordsHelper);
         _typeConverterFactory = new TypeConverterFactory(_parsingConfig);
-        _constantExpressionHelper = new ConstantExpressionHelper(_parsingConfig);
+        _constantExpressionHelper = ConstantExpressionHelperFactory.GetInstance(_parsingConfig);
 
         if (parameters != null)
         {

src/System.Linq.Dynamic.Core/Parser/ExpressionPromoter.cs

@@ -18,7 +18,7 @@ public class ExpressionPromoter : IExpressionPromoter
         public ExpressionPromoter(ParsingConfig config)
         {
             _numberParser = new NumberParser(config);
-            _constantExpressionHelper = new ConstantExpressionHelper(config);
+            _constantExpressionHelper = ConstantExpressionHelperFactory.GetInstance(config);
         }
 
         /// <inheritdoc />

src/System.Linq.Dynamic.Core/Parser/NumberParser.cs

@@ -16,7 +16,7 @@ public class NumberParser
         private static readonly char[] Qualifiers = { 'U', 'u', 'L', 'l', 'F', 'f', 'D', 'd', 'M', 'm' };
         private static readonly char[] QualifiersHex = { 'U', 'u', 'L', 'l' };
         private static readonly string[] QualifiersReal = { "F", "f", "D", "d", "M", "m" };
-        private ConstantExpressionHelper _constantExpressionHelper;
+        private readonly ConstantExpressionHelper _constantExpressionHelper;
 
         private readonly CultureInfo _culture;
 
@@ -27,7 +27,7 @@ public class NumberParser
         public NumberParser(ParsingConfig? config)
         {
             _culture = config?.NumberParseCulture ?? CultureInfo.InvariantCulture;
-            _constantExpressionHelper = new ConstantExpressionHelper(config ?? ParsingConfig.Default);
+            _constantExpressionHelper = ConstantExpressionHelperFactory.GetInstance(config ?? ParsingConfig.Default);
         }
 
         /// <summary>

src/System.Linq.Dynamic.Core/ParsingConfig.cs

@@ -3,6 +3,7 @@
 using System.Globalization;
 using System.Linq.Dynamic.Core.CustomTypeProviders;
 using System.Linq.Dynamic.Core.Parser;
+using System.Linq.Dynamic.Core.Util.Cache;
 
 namespace System.Linq.Dynamic.Core;
 
@@ -236,25 +237,7 @@ public IQueryableAnalyzer QueryableAnalyzer
     public bool DisallowNewKeyword { get; set; } = false;
 
     /// <summary>
-    /// Sets a Time-To-Live (TTL) for items in the constant expression cache to prevent uncontrolled growth. 
-    /// Items not accessed within this TTL will be expired, allowing garbage collection to reclaim the memory.
-    /// Default is 10 minutes.
+    /// Caches constant expressions to enhance performance. Periodic cleanup is performed to manage cache size, governed by this configuration.
     /// </summary>
-    public TimeSpan ConstantExpressionSlidingCacheTimeToLive { get; set; } = TimeSpan.FromMinutes(10);
-
-
-    /// <summary>
-    /// Configures the minimum number of items required in the constant expression cache before triggering cleanup. 
-    /// This prevents frequent cleanups, especially in caches with few items. 
-    /// A default value of null implies that cleanup is always allowed to run, helping in timely removal of unused cache items.
-    /// </summary>
-    public int? ConstantExpressionSlidingCacheMinItemsTrigger { get; set; } = null;
-
-
-    /// <summary>
-    /// Sets the frequency for running the cleanup process in the Constant Expression cache. 
-    /// By default, cleanup occurs every 10 minutes.
-    /// </summary>
-    public TimeSpan ConstantExpressionSlidingCacheCleanupFrequency { get; set; } = TimeSpan.FromMinutes(10);
-
+    public CacheConfig ConstantExpressionCacheConfig { get; set; } = new CacheConfig();
 }

src/System.Linq.Dynamic.Core/Util/Cache/CacheConfig.cs

@@ -0,0 +1,29 @@
+namespace System.Linq.Dynamic.Core.Util.Cache;
+
+/// <summary>
+/// Cache Configuration Options
+/// </summary>
+public class CacheConfig
+{
+    /// <summary>
+    /// Sets a Time-To-Live (TTL) for items in the constant expression cache to prevent uncontrolled growth. 
+    /// Items not accessed within this TTL will be expired, allowing garbage collection to reclaim the memory.
+    /// Default is 10 minutes.
+    /// </summary>
+    public TimeSpan TimeToLive { get; set; } = TimeSpan.FromMinutes(10);
+
+
+    /// <summary>
+    /// Configures the minimum number of items required in the constant expression cache before triggering cleanup. 
+    /// This prevents frequent cleanups, especially in caches with few items. 
+    /// A default value of null implies that cleanup is always allowed to run, helping in timely removal of unused cache items.
+    /// </summary>
+    public int? MinItemsTrigger { get; set; } = null;
+
+
+    /// <summary>
+    /// Sets the frequency for running the cleanup process in the Constant Expression cache. 
+    /// By default, cleanup occurs every 10 minutes.
+    /// </summary>
+    public TimeSpan CleanupFrequency { get; set; } = TimeSpan.FromMinutes(10);
+}

src/System.Linq.Dynamic.Core/Util/Cache/CacheContainer.cs

@@ -2,13 +2,13 @@
 
 internal struct CacheContainer<TValue> where TValue : notnull
 {
+    public TValue Value { get; }
+
+    public DateTime ExpirationTime { get; }
+
     public CacheContainer(TValue value, DateTime expirationTime) 
     {
         Value = value;
         ExpirationTime = expirationTime;
     }
-
-    public TValue Value { get; }
-
-    public DateTime ExpirationTime { get; }
 }

src/System.Linq.Dynamic.Core/Util/TaskUtils.cs

@@ -1,13 +1,14 @@
-namespace System.Linq.Dynamic.Core.Util;
+using System.Linq.Dynamic.Core.Validation;
+
+namespace System.Linq.Dynamic.Core.Util;
 
 internal static class TaskUtils
 {
     public static void Run(Action action)
     {
+        Check.NotNull(action);
 #if NET35 || NET40
-        System.Threading.ThreadPool.QueueUserWorkItem(_ => {
-            action?.Invoke();
-        });
+        System.Threading.ThreadPool.QueueUserWorkItem(_ => action.Invoke());
 #else
         System.Threading.Tasks.Task.Run(action);
 #endif

test/System.Linq.Dynamic.Core.Tests/Util/Cache/ThreadSafeSlidingCacheTests.cs

@@ -54,8 +54,8 @@ public void ThreadSafeSlidingCache_TestExpire()
         // Act
         cache.AddOrUpdate(1, "one");
 
-        var r = dateTimeUtilsMock.Object.UtcNow.AddMinutes(11);
-        dateTimeUtilsMock.SetupGet(d => d.UtcNow).Returns(r);
+        var newDateTime = dateTimeUtilsMock.Object.UtcNow.AddMinutes(11);
+        dateTimeUtilsMock.SetupGet(d => d.UtcNow).Returns(newDateTime);
 
         if (cache.TryGetValue(1, out var value))
         {
@@ -81,8 +81,8 @@ public void ThreadSafeSlidingCache_TestAutoExpire()
         cache.Count.Should().Be(1, $"Expected 1 items in the cache, only had {cache.Count}");
 
         // move the time forward
-        var r = dateTimeUtilsMock.Object.UtcNow.AddMinutes(11);
-        dateTimeUtilsMock.SetupGet(d => d.UtcNow).Returns(r);
+        var newDateTime = dateTimeUtilsMock.Object.UtcNow.AddMinutes(11);
+        dateTimeUtilsMock.SetupGet(d => d.UtcNow).Returns(newDateTime);
 
         // Trigger the cleanup, asking for non-existing key
         cache.TryGetValue(10, out var _);
@@ -124,8 +124,8 @@ public void ThreadSafeSlidingCache_TestMinNumberBeforeTests()
         cache.Count.Should().Be(1, $"Expected 1 items in the cache, only had {cache.Count}");
 
         // move the time forward
-        var r = dateTimeUtilsMock.Object.UtcNow.AddMinutes(11);
-        dateTimeUtilsMock.SetupGet(d => d.UtcNow).Returns(r);
+        var newDateTime = dateTimeUtilsMock.Object.UtcNow.AddMinutes(11);
+        dateTimeUtilsMock.SetupGet(d => d.UtcNow).Returns(newDateTime);
 
         // Trigger the cleanup, asking for non-existing key
         cache.TryGetValue(10, out var _);