unitsofmeasurement / indriya

JSR 385 - Reference Implementation
Other
115 stars 40 forks source link

Use exception catching, possibly unnecessary performance overhead #387

Closed fengyizhu closed 1 year ago

fengyizhu commented 1 year ago

tech.units.indriya.function.DefaultNumberSystem#narrow

        if(number instanceof BigDecimal) {

            final BigDecimal decimal = ((BigDecimal) number);
            try {
                BigInteger integer = decimal.toBigIntegerExact(); 
                return narrow(integer);
            } catch (ArithmeticException e) {
                return number; // cannot narrow to integer
            }
        }

why not just return BigDecimal?

fengyizhu commented 1 year ago
image
keilw commented 1 year ago

@fengyizhu What does this graph show, and is there a test suite you can share to replicate?

@andi-huber What was the BigInteger extraction done there? I understand isIntegerOnly() check simply does a Java type check, but why not apply isInteger() or try reuse the if(decimal.scale()<=0) part instead of trying the exception first?

fengyizhu commented 1 year ago

388

fengyizhu commented 1 year ago

@keilw Please See #388

andi-huber commented 1 year ago

Indeed, producing stacktraces just to do a check is costly. I merged https://github.com/unitsofmeasurement/indriya/pull/388

see also https://stackoverflow.com/questions/1078953/check-if-bigdecimal-is-an-integer-in-java

fengyizhu commented 1 year ago

@andi-huber @keilw Is there a plan and time to release a revised version?

keilw commented 1 year ago

The scope is here: https://github.com/unitsofmeasurement/indriya/milestone/10 but there's not a concrete time.