[incubator-kie-issues#83] Refactor InfixOpNode inside DMN core

[gitgabrio]

Fixes apache/incubator-kie-issues#83

This PR:

1. Remove the "behaviour" from the InfixOpNode
2. Delegate behaviour to InfixOperator and specific InfixExecutors
3. replace if logic with ClassIdentifierTuple -> BiFunction mapping

[gitgabrio]

Benchmark comparison using FEELInfixOpBenchmark
Before merge
results_pre.csv

After merge
results_post.csv

[tarilabs]

Thank you for sharing these results!
Seems to me this refactor "costed" you between 0 and 10% for the default case according to the data shared in #5621 (comment) .

(for these cases it's the CompiledButInterpreted subset of the variants as it's the default case for Interpreted at Runtime)

If that helps however, sometimes this preliminary conclusion is actually no-longer-true if you are able to push down the uncertainty error; for instance by running the benchmark for longer, or providing additional benchmark cases, etc.
I hope it was helpful to share!

[gitgabrio]

HI @tarilabs , thanks for help!
Reading the raw data, I see that for evaluateCompiledButInterpretedExpression there is a loss of performance.
But for evaluateCompiledJavaExpressionBenchmark there is a mix of result: in some cases a loss, in some cases a gain.
I'll look to improve that, and it would be extemely helpful if you could share some suggestion on what to benchmark exactly, namely:

1. I picked some infix expression from the code base, but I'm not sure if those are actually representative of the business domain
2. between the three different codepath executions I see in the benchmark, I'm not sure which is the most important, i.e. which one should I focus on the mostly

Could you please share them on the benchmark PR ?

@mariofusco @tarilabs @baldimir
That PR basically does two modifications

1. move code to specific packages/classes
2. remove the if-driven logic with a sort of "class-pattern-matching" based on the ClassIdentifierTuple and mapped BiFunctions (that also required the implementation and instantiation of EvaluatedParameters)

Compared with the "if" choice, I'm pretty sure that the second point is more expensive, but removing that "if" was one of the goal: do you have some idea/suggestion for a more efficient implementation ?
The requirement is that for any given pair of class types, a specific method should be invoked.

[tarilabs]

I'll look to improve that, and it would be extemely helpful if you could share some suggestion on what to benchmark exactly, namely:

1. I picked some infix expression from the code base, but I'm not sure if those are actually representative of the business domain
2. between the three different codepath executions I see in the benchmark, I'm not sure which is the most important, i.e. which one should I focus on the mostly

CompiledButInterpreted is usually your "non-negotiable", because is the one where you have a DMN engine at runtime initialized with a DMN model, and you evaluate multiple times that DMN model with different set of inputs. So it's the one you typically exercise the most on the field.

The [fully] Interpreted have a play with the KIE Sandbox style of interacting with the engine, because every time with JIT DMN you compile the .dmn and evaluate with the inputs (on a tangent here we experimented with Caffeine to cache the result in JIT DMN so that if you are testing the same DMN model, you don't need to recompile it, but unf. it was still an experimental extension, not sure if this can be revisited now). However the performance of evaluation here are clearly negligible if compared to most time is typically compiling the model.

The CompiledJava kicks in only if you are emitting the compilation and using that (not the default, see comment here).

I would also suggest to take a look at algebric expression benchmarks (iirc we had a few) so to have a more "macro"-benchmark for reference in this scope?

Hope this helps!

[mariofusco]

@gitgabrio Personally I don't have a strong opinion on which of the 2 versions is more readable or maintainable, it is really a matter of tastes and I don't want to argue on them.

Regarding the performances, if we're serious about them my suggestion is not only benchmarking more extensively the effects of this change, as also asked by @tarilabs, but also running some proper profiling sessions to better figure out where we are consuming time and how we can improve. For instance I have the feeling that creating a new instance of that ClassIdentifierTuple at each and every evaluation of a numeric expression and use it as the key for the map containing the function to be executed on its operands is a non-negligible waste and could be easily avoided, but again this is just a wild guess and I'd prefer to make changes driven by the evidence of a profiling session.

[gitgabrio]

@mariofusco @tarilabs @baldimir
FYI
apache/incubator-kie-issues#780
apache/incubator-kie-issues#781

[gitgabrio]

@mariofusco
For instance I have the feeling that creating a new instance of that ClassIdentifierTuple at each and every evaluation of a numeric expression and use it as the key for the map containing the function to be executed on its operands is a non-negligible waste and could be easily avoided, this is also my impression, but it does not comes to my mind an "easy way" to avoid that : could you share some raw suggestion ?

[mariofusco]

@gitgabrio @tarilabs @baldimir I'm sorry but I did some very preliminary checks and it seems that this commit is worsen performances of 2x if not more, at least in CompiledButInterpreted use case. In fact as I reported here the original benchmark was flawed. I created another, simplistic but more correct benchmark like the following:

package org.drools.benchmarks.dmn.feel;

import org.kie.dmn.feel.FEEL;
import org.kie.dmn.feel.lang.CompiledExpression;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.Fork;
import org.openjdk.jmh.annotations.Measurement;
import org.openjdk.jmh.annotations.OutputTimeUnit;
import org.openjdk.jmh.annotations.Param;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.Warmup;

import java.util.Map;
import java.util.concurrent.TimeUnit;

@Warmup(iterations = 5)
@Measurement(iterations = 10)
@OutputTimeUnit(TimeUnit.MILLISECONDS)
@Fork(1)
@State(org.openjdk.jmh.annotations.Scope.Thread)
public class FEELInfixOpBenchmark {

    private FEEL feelInterpreted;

    private CompiledExpression compiledButInterpretedExpression;

    private Map<String, Object> map;

    @Param({"String", "int"})
    private String argsType;

    @Setup
    public void setup() {
        feelInterpreted = FEEL.newInstance();
        compiledButInterpretedExpression = compileInterpretedExpression("a + b");
        map = argsType.equals("String") ? Map.of("a", "1", "b", "2") : Map.of("a", 1, "b", 2);
    }

    private CompiledExpression compileInterpretedExpression(String expression) {
        return feelInterpreted.compile(expression, feelInterpreted.newCompilerContext());
    }

    @Benchmark
    public Object eval() {
        return feelInterpreted.evaluate(compiledButInterpretedExpression, map);
    }
}

If I run this benchmark against 8.44.0.Final I get the following results:

Benchmark                  (argsType)   Mode  Cnt     Score    Error   Units
FEELInfixOpBenchmark.eval      String  thrpt   10  3866.743 ± 30.992  ops/ms
FEELInfixOpBenchmark.eval         int  thrpt   10  3367.492 ± 11.529  ops/ms

while doing the same on the latest snapshot which includes this change I get:

Benchmark                  (argsType)   Mode  Cnt     Score    Error   Units
FEELInfixOpBenchmark.eval      String  thrpt   10  1676.167 ± 10.623  ops/ms
FEELInfixOpBenchmark.eval         int  thrpt   10  1748.573 ±  9.847  ops/ms

This definitively deserves more investigation and a proper profiling session (I'm willing to help with this), but either we find a way to restore the former performance levels or I strongly suggest to revert this commit before the deployment of the next stable release.

[gitgabrio]

@mariofusco
Thanks for that.
I'm currently working on apache/incubator-kie-issues#780 to provide and isolate benchmark at different leveles, i.e. the InfixOpNode as a whole and the different InfixExecutors

[mariofusco]

Quick update: the problem seems indeed in the use of that Map to retrieve the right function to be applied for the types of operand at hand: