Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
aviita opened this issue Aug 9, 2021 · 4 comments · Fixed by adletec/sonic#1 · May be fixed by #76
Open

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

aviita opened this issue Aug 9, 2021 · 4 comments · Fixed by adletec/sonic#1 · May be fixed by #76

Comments

@aviita
Copy link

aviita commented Aug 9, 2021

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
Copy link
Author

aviita commented Aug 10, 2021

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
Copy link
Author

aviita commented Aug 10, 2021

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 added a commit to aviita/Jace that referenced this issue Aug 10, 2021
@aviita
Fix performance issue with CaseSensitivity enabled
6353202 
When CaseSensitive is set to true from JaceOptions, performance should
be significantly better than without case sensitivity. Case sensitivity
setting was not passed to `FunctionRegistry` and `ConstantRegistry`
constructors, which caused them to do extra lower case conversions in
case variable dictionary was passed to the formula. Fixes [issue pieterderycke#75][1].

New benchmark was created to verify the fix. Benchmark needs to have
variables which are provided to `CalculationEngine.Calculate()`, so
VerifyVariableNames() gets called, which causes the extra calls to
`ToLowerFast()`.

Below results show ~200 ms improvement when Case Sensitive is true.
Results table was created by running benchmark separately with
and without fix and copying results to one table.

| Engine     | Case Sensitive | 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 |

Additionally:
- Unit test which tests case sensitivity enabled throws when variable
  case does not match
- Benchmark uses dictionary copy constructor to avoid throwing due to
  dictionary being modified with constants and then failing on
  `VerifyVariableNames()`.

[1]: pieterderycke#75
@aviita aviita linked a pull request Aug 10, 2021 that will close this issue
Open
@aviita
Copy link
Author

aviita commented Aug 10, 2021

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

@aviita
Copy link
Author

aviita commented Aug 10, 2021

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

@vlow vlow mentioned this issue Apr 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
1 participant
@aviita