jeffhain / jafama

A (Strict)FastMath class with 1e-15ish accuracy.
81 stars 12 forks source link

Please consider making your optimized implementation of Math.log accessible via a separate method, analogous to FastMath.logQuick. #5

Closed jhauffa closed 4 years ago

jhauffa commented 4 years ago

In a particular application (HMM parameter fitting in log-space using the Baum-Welch algorithm), I am seeing a small, but noticeable performance benefit from using your optimized logarithm method FastMath.log. However, jafama delegates calls to FastMath.log to Math.log unless a system property "jafama.fastlog" is set to "true". There are two issues with this design choice:

1) The code I am writing is part of a library, so there seems to be no way for me to reliably set the property before jafama is loaded. I can only document the requirement and trust the users to do the right thing, which is somewhat unsatisfying.

2) In the (admittedly unlikely) case that someone uses my library in conjunction with another library, which also depends on jafama, setting the global property will affect the behavior of both libraries.

It would be much appreciated if you introduced a new method (say, FastMath.logOptimized), that implements the same alogrithm as FastMath.log, but, like FastMath.logQuick, only delegates to Math.log if "jafama.usejdk" is "true".

jeffhain commented 4 years ago

Hi.

Sorry for the late reply, I don't often check my github, it's usually quite calm :)

Letting library user choose the option in some launching script, or documenting it for the end user, allows to choose the most appropriate configuration depending on the context (redefined log might be the fastest on your platform, but not on end user's platform).

Why do you want not to affect both libraries? Isn't it desirable to be able to use the fastest log implementation everywhere?

logQuick() : xxxFast() and xxxQuick() methods allow for large speed-ups by reducing validity range and accuracy a lot, these are different beasts than the regular Math-like methods.

I worry that cluttering FastMath API with multiple implementations for the same semantics could confuse the user, and not allow for easy global configuration as described above. Also, FastMath.log() is used in other FastMath methods (pow(), acosh(), etc.), and to be consistent we should also provide variants for these methods depending on which log() they would use: that would start to get complicated.

I don't think that using a property at launch time for implementation switches is such a bad design choice. It is done in HotSpot for a similar case, which is BigInteger multiplication: the "UseMultiplyToLenIntrinsic" option allows to switch between pure Java or intrinsic implementation (hand-coded assembly, about twice faster).

If you still really want to stick to redefined log(), in spite of all the above, you can still just copy-paste it into your code, it's not that big and it's a bit what open source is for ;) If your values are not pathological, you could then make it even faster, by removing the "0/infinity/NaN" checks.

jhauffa commented 4 years ago

Why do you want not to affect both libraries? Isn't it desirable to be able to use the fastest log implementation everywhere?

The case I am concerned about is the following: Consider a hypothetical scenario where a third party application "A" depends on my HMM library and some other library "LibX". Both my library and LibX depend on jafama. I have tested my library with "jafama.fastlog" set to true and false, and know that it works fine with either implementation of FastMath.log. Therefore I recommend that users of my library set jafama.fastlog=true to benefit from the faster implementation. LibX, however, requires the higher accuracy you get with jafama.fastlog=false. If the author of A follows my recommendation, this will break LibX. In the worst case, the breakage is subtle and won't be detected immediately, making it difficult to trace it back to the setting of jafama.fastlog.

I don't think that using a property at launch time for implementation switches is such a bad design choice. It is done in HotSpot for a similar case, which is BigInteger multiplication: the "UseMultiplyToLenIntrinsic" option allows to switch between pure Java or intrinsic implementation (hand-coded assembly, about twice faster).

For the record, I also don't think it's bad design; it's a quite clever way of letting users choose between different implementations while enabling the compiler/JVM to optimize out the indirection. I am worried about unintended side effects if the implementations are not completely functionally equivalent, though.

If you still really want to stick to redefined log(), in spite of all the above, you can still just copy-paste it into your code, it's not that big and it's a bit what open source is for ;) If your values are not pathological, you could then make it even faster, by removing the "0/infinity/NaN" checks.

This is what I am currently doing - with proper attribution, of course. Works for me, so feel free to close this issue.

jeffhain commented 4 years ago

LibX, however, requires the higher accuracy you get with jafama.fastlog=false.

Then it would be an issue with LibX relying on jafama's log, which is not required to be as accurate as Math.log(), where Math.log() accuracy is needed. If I found an algorithm that was a bit less accurate than Math.log(), but always noticeably faster, I would remove the "jafama.fastlog" switch and always use this algorithm, as done for almost all other methods (as long as "jafama.usejdk" is false).

Cheers, Jeff