Use the new methods introduced in Java 1.8 #139
Conversation
…rUnsigned and divideUnsigned (both int and long)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #139 +/- ##
============================================
- Coverage 99.23% 99.22% -0.01%
+ Complexity 1828 1804 -24
============================================
Files 70 70
Lines 4808 4776 -32
Branches 896 884 -12
============================================
- Hits 4771 4739 -32
Misses 10 10
Partials 27 27 ☔ View full report in Codecov by Sentry. |
|
I would be wary of this simplification without a performance test. In the Numbers class the int methods do not use long arithmetic. The long methods do not use BigInteger. This is unlike those methods in my JDK 8 source code which do and may be slower. A quick check in JDK 21 finds this is still not an intrinsic method [1]. Note that the Numbers methods are based on the Hacker's Delight book which is not free, thus it is not easy to check the However, there is frequent use of Hacker's Delight in the JDK source. So I wonder why they have not use this trick here. I can create a quick JMH benchmark to test the Numbers methods against the JDK. The int method may not be faster as long divide should be supported on most hardware. But avoiding BigInteger divide may be noticeable. Alex [1] VM Intrinsics Explorer - HotSpot Intrinsics for OpenJDK21 |
|
If the reason we don't use the JDK methods is performance, this should be explicitly noted in the source so future maintainers are aware. |
|
I agree. ArithmeticUtils predates JDK 8. I imagine it was ported from Math to Numbers without knowing that JDK 8 had equivalent methods. I'll do a performance test, then we can either merge this PR, or update the code with comments on the performance vs the JDK methods. |
|
Here are some benchmark results. JDK variants are prefixed with the class that implements the method. TLDR; the JDK is always faster for the int methods. It can be dramatically slower for the long methods. They fixed this in JDK 17. It seems that performing the int divide using conversion to a long is faster than the method to avoid long arithmetic. But the BigInteger divide was only fixed in JDK 17. I would recommend: dropping the int divide variants and delegating to the JDK methods; and keeping the long divide variants. A note can be added to the javadoc that the equivalent method in the JDK changed from JDK 11 to 17 to avoid BigInteger arithmetic. I do not think we should add a deprecated notice to the method until all supported versions of the JDK are comparable in speed. Note: The int results would be more interesting on a CPU that does not support native long division. |
|
Just browsed the JDK 17 source code for the long methods. These use a method from Hacker's Delight (2nd ed). So the Hacker's Delight (presumed 1st ed) method in ArithmeticUtils has been improved. Without access to the book, and a license compatibility check, we cannot use the updated version. But the original is still 15-25x faster than the JDK 8/11 version using BigInteger. |
|
Added the benchmark to master. Run using: |
|
I've obtained a copy of Hacker's Delight 2.0. Section 9.3 contains details of how to implement unsigned divide. The unsigned modulus is not provided but I have reworked the current code based on the divide code and the old remainder code. JDK 17 So the new long version is approximately 25-30% faster and matches the JDK. JDK 21 (Long.remainderUnsigned and Long.divideUnsigned are intrinsic methods from JDK 19) Here the JDK is faster due to the intrinsic methods (or some other optimisation - I didn't check the source code). I have updated the int version to delegate to Integer.remainderUnsigned/divideUnsigned with the rather obvious result that it is now as fast as the JDK (which uses long arithmetic): Changes added to master. |
No description provided.