Improve performance of DefaultNumberSystem.narrow(BigDecimal)

[mapeters985]

I recently upgraded to indriya version 2.0.2, and found that some conversions were very slow as a result, specifically RationalConverters. I traced this to the way that DefaultNumberSystem.narrow(Number) handles BigDecimal parameters.

For BigDecimals, the DefaultNumberSystem.narrow method calls BigDecimal.toBigIntegerExact() and catches the ArithmeticException that is thrown if the number has a nonzero fractional part. This throwing/catching exceptions can be slow when tons of conversions on fractional values is done.

Checking if the BigDecimal is an integer value first and then converting it, instead of relying on exceptions, appears to speed up this operation. The BigDecimal block of that method could be replaced with this:

if (number instanceof BigDecimal) {
    BigDecimal decimal = (BigDecimal) number;
    decimal = decimal.stripTrailingZeros();
    if (decimal.scale() <= 0) {
        BigInteger integer = decimal.toBigInteger();
        return narrow(integer);
    }
    return number;
}

That logic is taken from this Stack Overflow post: https://stackoverflow.com/questions/1078953/check-if-bigdecimal-is-an-integer-in-java

Here is a very basic, un-offical performance test that indicates around a 3-4x performance improvement with this change on my system: TestMultiplyConverterPerf.java.txt

[andi-huber]

Possibly a duplicate of https://github.com/unitsofmeasurement/indriya/issues/387 and fixed in 2.1.4.

[mapeters985]

Thanks Andi, this is a duplicate of that. But I don't see that change in the master branch here or in the 2.1.4 sources jar on maven: https://repo1.maven.org/maven2/tech/units/indriya/2.1.4/indriya-2.1.4-sources.jar

Am I looking in the wrong spot?

[keilw]

No, unfortunately the PR #388 was buggy and it broke numereous JUnit tests. The assumption or simplification looks immature, so it was disregarded from 2.1.4.

@mapeters985 Not sure, if the extra line compared to that PR really helps enough, and all tests keep passing? Feel free to fork and try it out yourself, but please ensure, all tests pass before proposing another PR.

[mapeters985]

@keilw Thanks. Are there instructions on how to run the unit tests somewhere? I'd like to try them out with my modified version of the fix.

[keilw]

Either mvn clean install on the top level repo or if you just want to run tests after you already built everything mvn test. Of course the narrowing business has side-effects like #384 but as the conversion via system units happens based on the unit, not the number type the two are not directly related.

So I offer it as a candidate for 2.2 but won't schedule it into any iteration yet. So far there are two more sprints of 2 weeks each. Ideally we should be done by April and propose a Maintenance Draft Review then, so the MR2 could be available around World Metrology Day on May 20.

[mapeters985]

Thanks, the unit tests passed with my suggested change, so I will go ahead and propose this as a PR.

[mapeters985]

@keilw Do I have to become a JSR-385 member to submit a pull request? I've tried going to that JSR while logged into my JCP account, but I don't see any option to apply for membership.

[keilw]

@mapeters985 So you are a JCP member already?

[andi-huber]

I guess we can close this one as of https://github.com/unitsofmeasurement/indriya/pull/405.

[keilw]

Sure, thanks