pieterderycke / Jace

Jace.NET is a calculation engine for the .NET platform.
MIT License
436 stars 94 forks source link

JaceOptions case sensitivity does not get applied to FunctionRegistry or ConstantRegistry on engine constructor #75

Open aviita opened 3 years ago

aviita commented 3 years ago

FunctionRegistry and ConstantRegistry ignore JaceOptions.CaseSensitive, which has performance implications as there are lots of unnecessary calls to ToLowerFast().

        public CalculationEngine(JaceOptions options)
        {
            this.executionFormulaCache = new MemoryCache<string, Func<IDictionary<string, double>, double>>(options.CacheMaximumSize, options.CacheReductionSize);
            this.FunctionRegistry = new FunctionRegistry(false);
            this.ConstantRegistry = new ConstantRegistry(false);
            this.cultureInfo = options.CultureInfo;
            this.cacheEnabled = options.CacheEnabled;
            this.optimizerEnabled = options.OptimizerEnabled;
            this.caseSensitive = options.CaseSensitive;

Even after disabling case sensitivity, it gets checked by both ConstantRegistry.IsConstantName() and FunctionRegistryIsFunctionName().

image

This does not seem to break the case sensitivity it self as even this test case I wrote to CalculationEngineTests passes:

        [TestMethod]
        public void TestCalculateFormulaWithCaseSensitiveThrows()
        {
            Dictionary<string, double> variables = new Dictionary<string, double>();
            variables.Add("var1", 1);
            variables.Add("var2", 1);

            CalculationEngine engine = new CalculationEngine(new JaceOptions { CaseSensitive = true });
                //CultureInfo.InvariantCulture, ExecutionMode.Compiled, false, false, false);
            var ex = AssertExtensions.ThrowsException<VariableNotDefinedException>( () => engine.Calculate("VaR1*vAr2", variables));
            Assert.AreEqual("The variable \"VaR1\" used is not defined.", ex.Message);
        }
aviita commented 3 years ago

I also ran the Jace benchmarks as those seemed to allow testing performance with and without case sensitivity. The tests are neclecting to test the basic use case though that does not use the FormulaBuilder. FormulaBuilder seems to have been implemented as expected. The single benchmark does not take the input variable dictionary that are part of the problem here.

I created some new benchmark cases to see the performance impact. The rightmost column ("Total Duration (no fix)") has results before fix and the one before that ("Total Duration (fix)") has fix to engine constructor applied. An improvement of ~200 ms can be observed in cases where case sensitivity is set to True.

Engine Case Sensitive Formula Iterations per Random Formula Total Iteration Total Duration (fix) Total Duration (no fix)
Interpreted False something2 - (var1 + var2 * 3)/(2+3) 1000000 00:00:01.6005267 00:00:01.5919016
Interpreted True something2 - (var1 + var2 * 3)/(2+3) 1000000 00:00:00.6069845 00:00:00.8390435
Compiled False something2 - (var1 + var2 * 3)/(2+3) 1000000 00:00:01.5865326 00:00:01.5770084
Compiled True something2 - (var1 + var2 * 3)/(2+3) 1000000 00:00:00.5930012 00:00:00.8189981
aviita commented 3 years ago

Here are also the full excel files before and after the fix. Note that only the benchmarks with formula "something2 - (var1 + var2 * 3)/(2+3)" are affected when case sensitivity is True.

jace_benchmark_var_dict_no_fix.xlsx jace_benchmark_var_dict_fix.xlsx

aviita commented 3 years ago

@pieterderycke Created a PR #76 to fix the issue.

aviita commented 3 years ago

This bug can also cause some extra memory traffic. While this is likely Gen 0 Heap, it surely would not hurt to get it removed too. The fix should apply to this one too.

image