value method reduction discussion

[JaredDavis22]

Hi -

My off label usage has a higher than normal hit for accessing the variables.

What are the pros/cons of changing the code to do one value() call per operation per parameter vs one per each instance check?

Currently every is{X} call does a value call.

public Object lessThan(final Object a, final Object b) {
		if (isBool(a) && isBool(b)) return lessThan(bool(a), bool(b));
		if (isStr(a) && isStr(b)) return lessThan(str(a), str(b));
		if (isI(a) && isI(b)) return lessThan(i(a), i(b));
		if (isL(a) && isL(b)) return lessThan(l(a), l(b));
		if (isF(a) && isF(b)) return lessThan(f(a), f(b));
		if (isD(a) && isD(b)) return lessThan(d(a), d(b));
		if (isBI(a) && isBI(b)) return lessThan(bi(a), bi(b));
		if (isBD(a) && isBD(b)) return lessThan(bd(a), bd(b));
		return null;
	}

private boolean isBool(final Object o) { return is(o, Boolean.class); }
private boolean is(final Object o, final Class<?> c) {
		return c.isInstance(value(o));
	}

vs

public Object lessThan(final Object aa, final Object bb) {
		Object a = value(aa);
		Object b = value(bb);
		if (isBool(a) && isBool(b)) return lessThan(bool(a), bool(b));
		if (isStr(a) && isStr(b)) return lessThan(str(a), str(b));
		if (isI(a) && isI(b)) return lessThan(i(a), i(b));
		if (isL(a) && isL(b)) return lessThan(l(a), l(b));
		if (isF(a) && isF(b)) return lessThan(f(a), f(b));
		if (isD(a) && isD(b)) return lessThan(d(a), d(b));
		if (isBI(a) && isBI(b)) return lessThan(bi(a), bi(b));
		if (isBD(a) && isBD(b)) return lessThan(bd(a), bd(b));
		return null;
	}

private boolean is(final Object o, final Class<?> c) {
		return c.isInstance(o);
	}

[ctrueden]

@JaredDavis22 I benchmarked the lessThan function with and without your proposed change. Here are the results:

  Warmup: 200,000 calls | Measure: 2,000,000 calls

=== DefaultStackEvaluator - lessThan(Variable, Variable) ===
	                       CURRENT                   PROPOSED
  Type               ns/op          ops/sec    ns/op          ops/sec
  ------------------------------------------   -----------------------
  Boolean             44.6       22,413,911     53.6       18,644,761
  String              48.9       20,467,700     47.8       20,931,807
  Integer             59.8       16,717,810     54.0       18,505,039
  Long                84.9       11,785,002     72.3       13,828,700
  Float              127.9        7,817,979    101.3        9,870,592
  Double             205.3        4,872,038    137.9        7,253,047
  BigInteger         562.7        1,777,164    171.9        5,815,817
  BigDecimal         448.3        2,230,737    211.0        4,738,682

=== DefaultTreeEvaluator - lessThan(Variable, Variable) ===
                           CURRENT                   PROPOSED
  Type               ns/op          ops/sec    ns/op          ops/sec
  ---------------------------------------------------------------------
  Boolean            125.7        7,957,421    122.0        8,196,157
  String              73.0       13,699,744     49.4       20,260,194
  Integer             97.9       10,213,055     67.2       14,881,069
  Long               153.1        6,529,671     98.4       10,163,443
  Float              196.7        5,084,744    123.2        8,114,269
  Double             286.0        3,496,804    185.5        5,392,164
  BigInteger         609.1        1,641,881    231.6        4,318,313
  BigDecimal         532.5        1,877,828    262.1        3,815,768

So your proposal improves time performance in many/most cases. I'm going to play with this some more, and will follow up here again after I have more results to report.

[ctrueden]

@JaredDavis22 Can you give the latest on main a try and see how it goes for you?

[JaredDavis22]

Thank you for making the changes! I'll take a look tomorrow.

PS Are you ok with throwing UnsupportedOperationException for the unimplemented operations? I will do a PR if you approve of the approach.

[JaredDavis22]

PS Did you benchmark the (non variable, non variable) path?

[ctrueden]

Are you ok with throwing UnsupportedOperationException for the unimplemented operations? I will do a PR if you approve of the approach.

Good idea. It was super easy to do, so I just did it.

PS Did you benchmark the (non variable, non variable) path?

I did not. More benchmarks would be valuable. If you wish, you can model them after the LessThanBenchmarkTest—more test methods in that class and/or new FooBenchmarkTest classes as appropriate.

[JaredDavis22]

Thanks again.

Are you ok with jmh with profile test for the benchmarks?

The other item on my agenda is to look at what happens when operands have different numeric types. eg BigDecimal * double etc. I did not dig on it yet. Will it return null? Perhaps that should also throw an UOE (only with strict mode?) vs return null.

[JaredDavis22]

and...

default Object execute(final Operator op, final Object... args)

It probably has 0 impact on speed, but re-order the list a little bit? Move the unimplemented items to the bottom, +-*/ near the top, bitwise stuff lower, ...?

I'm a fan of having the code throw an exception vs returning something that may make the error harder to find. Also a fan of having the details in the exception message so it is easier to resolve. Throwing exceptions may break existing code (which handled the null with a different approach.)

More than willing to do PRs. Please let me know if you agree with the approach.

Add boolean throwOnUnknown (vs strict) then use that to return null or throw? Default it to true?

[ctrueden]

Are you ok with jmh with profile test for the benchmarks?

Sure, that would be great!

what happens when operands have different numeric types. eg BigDecimal * double etc.

It's all driven by what the isFoo methods identify as compatible with each type. For BigDecimal and Double, the Double will return true when passed to isBD, because:

private boolean isBD(final Object o) { return is(o, BigDecimal.class) || isBI(o) || isD(o); }

These "is" methods are really more like "canImplicitlyCastTo", at least for numeric types.

$ jgo org.scijava:parsington
> bi = 9999999999999999999999999
bi : org.scijava.parsington.Variable = 9999999999999999999999999 : java.math.BigInteger
> bd = 9999999999999999999 * 5.5
bd : org.scijava.parsington.Variable = 54999999999999999994.5 : java.math.BigDecimal
> bi * bd
549999999999999999944999945000000000000000005.5 : java.math.BigDecimal
> bd * bi
549999999999999999944999945000000000000000005.5 : java.math.BigDecimal

The above works because isBI(bd) returns false, but isBD(bi) returns true, so the "both numbers smell like BigDecimal" path is what ends up getting followed regardless of operand order.

But that's not to say that behavior is always intuitive 😅

> 1234567890987654321 * 5.5
6.7901234004320993E18 : java.lang.Double
> 12345678900987654321 * 5.5
67901233955432098765.5 : java.math.BigDecimal

(long * double: lossy! BigInteger * double: lossless!)

> 9999999999999 - 9999999999998
1 : java.lang.Long

(Result is small enough to fit into a byte? Doesn't matter, it's a long.)

Feel free to surface such oddities as their own issues if they are problematic for you, and we can discuss if different behavior would be better in each case.

It probably has 0 impact on speed, but re-order the list a little bit? Move the unimplemented items to the bottom, +-*/ near the top, bitwise stuff lower, ...?

I prefer the order of methods in an implementing class to match those of the corresponding interface. And method implementations are ordered from "nearest" to "most ancestral" interface. It's been awhile, but IIRC, the order of the StandardEvaluator (and therefore AbstractStandardEvaluator) code was chosen to reflect operator precedence, from highest to lowest, aligned with how Java does it (even though how Java does it can be confusing in some cases—consistency!).

I'm a fan of having the code throw an exception vs returning something that may make the error harder to find. Also a fan of having the details in the exception message so it is easier to resolve. Throwing exceptions may break existing code (which handled the null with a different approach.)

Agreed. I don't think the behavioral change around unimplemented operations is that disruptive, though, so I lean toward staying at 3.x according to SemVer, because we aren't actually removing any API.

More than willing to do PRs. Please let me know if you agree with the approach.

If you want to add a more descriptive message to each UnsupportedOperationException, go for it. I suggest to make it as DRY as possible though, probably with a helper method like private void unsupported(final String op) { throw new UnsupportedOperationException("The " + op + " operation is unsupported with this evaluator") or whatever wording you think is best.

Add boolean throwOnUnknown (vs strict) then use that to return null or throw? Default it to true?

Not sure what you're proposing with this—a toggle of return null vs throw? If so: should we reuse strict for this?

[JaredDavis22]

Thanks for the information.

I've created a fork where I did the usual error of premature optimization. I'm trying to reduce the runtime. The changes are ugly in spots. I'll run some benchmarks over the next couple of days and report back with the results.

https://github.com/JaredDavis22/parsingtonFork/tree/throwvsnullreturn

[ctrueden]

I'm trying to reduce the runtime.

It sounds like optimal performance is critical for your use case? Are your expressions being evaluated more than once? If so, have you considered compiling them to bytecode and running them that way, to benefit more from the JIT?

Edit: The other thing you can do, if you haven't already tried, is ask Claude or whichever AI to identify potential performance wins across Parsington—I have actually never done that, since Parsington's implementation predated the rise of LLMs. I expect that would find a few impactful changes we could make.

[JaredDavis22]

Hi - I changed the static standard operators list creation based on your suggestion. Thanks.

I will probably end up with a native executable. Time will tell.

The exception text seems to be working ok so far. The parameters type would be good to add at a later time.

        System.out.println(e.evaluate("vv=true"));
        System.out.println(e.evaluate("sqrt(300.1+vv)"));

java.lang.UnsupportedOperationException: add parameters not supported. av=300.1 bv=true
vs
java.lang.NullPointerException: Cannot invoke "Object.getClass()" because "b" is null

[JaredDavis22]

Thoughts on adding a getOrDefault(Object key, V defaultValue)?

The variable strict is always true for me. I find the need for a non exception throwing version when the code is an an error path.