Attachments

Activity

Andy, thanks for letting me know about this issue. I had no idea it's been sitting here in JIRA for months.

1. These numbers really do need to be converted to BigInt, not BigInteger.

2. There's nothing inherently problematic with the call to bigint. In my tests, it appears that this patch doesn't meaningfully improve the performance of floor or ceiling, only round. That's because it's not the call to bigint that is slow, but in round, you've used a call to .setScale with ROUND_HALF_UP, and that's faster than the method used in the code (adding 0.5M and then flooring).

3. Even though .setScale with ROUND_HALF_UP is faster than the current code, it does not return the correct results for negative numbers like -4.5M. The goal is to make the behavior match Math/round on doubles. For some reason, BigDecimal's ROUND_HALF_UP does the wrong thing for negative numbers (rounding up in magnitude, rather than rounding up in the sense of making it more positive).

So this patch is a no-go. The changes to floor and ceiling return BigInteger rather than BigInt and when modified to return BigInt, do not yield speed benefits. The change to round does yield a speed benefit, but the results are incorrect for negative numbers halfway between integers.

If you find a faster way to round decimal numbers than the existing mechanism that yields the correct results for negative numbers, let me know.

Mark Engelberg
added a comment - 02/Jan/14 12:36 AM Andy, thanks for letting me know about this issue. I had no idea it's been sitting here in JIRA for months.
1. These numbers really do need to be converted to BigInt, not BigInteger.
2. There's nothing inherently problematic with the call to bigint. In my tests, it appears that this patch doesn't meaningfully improve the performance of floor or ceiling, only round. That's because it's not the call to bigint that is slow, but in round, you've used a call to .setScale with ROUND_HALF_UP, and that's faster than the method used in the code (adding 0.5M and then flooring).
3. Even though .setScale with ROUND_HALF_UP is faster than the current code, it does not return the correct results for negative numbers like -4.5M. The goal is to make the behavior match Math/round on doubles. For some reason, BigDecimal's ROUND_HALF_UP does the wrong thing for negative numbers (rounding up in magnitude, rather than rounding up in the sense of making it more positive).
So this patch is a no-go. The changes to floor and ceiling return BigInteger rather than BigInt and when modified to return BigInt, do not yield speed benefits. The change to round does yield a speed benefit, but the results are incorrect for negative numbers halfway between integers.
If you find a faster way to round decimal numbers than the existing mechanism that yields the correct results for negative numbers, let me know.