-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Move sloppySin into SloppyMath from GeoUtils #14516
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Ankit Jain <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree with the such changes without supporting benchmarks, sorry. Especially it is bad news to just replace 3 functions (sin,cos,asin) all at once in YOLO fashion.
Thanks for raising these concerns. The intention was to just remove custom |
It will mostly impact geo sorting. But replacing the fast SloppyMath calculation here with slower equivalents from OpenJDK won't buy you any improved error rate: precision is purposefully truncated to ensure stable sorts: https://github.com/apache/lucene/blob/main/lucene/core/src/java/org/apache/lucene/util/SloppyMath.java#L73-L74 |
Signed-off-by: Ankit Jain <[email protected]>
Signed-off-by: Ankit Jain <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
But the benchmark is not useful, as it was written as a test. Tests don't run normally, in particular without java C2 compiler, so trying to make benchmark via test is not useful and biased in many ways. If you want to make a microbenchmark, please use |
See guide here: https://github.com/apache/lucene/blob/main/help/jmh.txt |
Signed-off-by: Ankit Jain <[email protected]>
Signed-off-by: Ankit Jain <[email protected]>
This reverts commit 788564c.
This reverts commit cb583a2.
Signed-off-by: Ankit Jain <[email protected]>
Signed-off-by: Ankit Jain <[email protected]>
JMH benchmark comparison for just the sloppySin / standardSin comparison:
|
JMH benchmark comparison for just the FromPointDistanceSloppySin / FromPointDistanceStandardSin comparison:
|
Signed-off-by: Ankit Jain <[email protected]>
Signed-off-by: Ankit Jain <[email protected]>
Thanks @rmuir for the suggestion. I have added couple of jmh benchmarks for this, and they clearly show, probably not the right change. While the slowdown for I have just moved |
* @return the sine of the argument. | ||
* @see Math#sin(double) | ||
*/ | ||
public static double sin(double a) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to promote this from an experimental API in GeoUtils to a non-experimental public API here?
If we really want to do this, let's keep in line with the existing methods in the file, and provide accuracy bounds and tests?
Personally I think an easier solution would be to "demote" it from GeoUtils to a package-private static method: seems like it is only used by Rectangle and it doesn't need to be public. This would be easier than "properly supporting it" IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I think an easier solution would be to "demote" it from GeoUtils to a package-private static method: seems like it is only used by Rectangle and it doesn't need to be public. This would be easier than "properly supporting it" IMO.
Thanks @rmuir for the suggestions. While I agree it is easier this way, but IMO:
- It is better to have related stuff in single place, even if we are using only once currently
- The benchmarks will be unable to use if it is private, and I feel they do add some value, in case some else like myself wonders if we really need sloppySin!?
Do we really need to promote this from an experimental API in GeoUtils to a non-experimental public API here?
If we really want to do this, let's keep in line with the existing methods in the file, and provide accuracy bounds and tests?
Had not realized myself that this promotes to non-experimental API. I have been bit sloppy with this SloppyMath stuff! :( Thanks for pointing and patiently reviewing so far.
That being said, I don't see any real harm with sloppySin
getting promoted to SloppyMath
. I have added accuracy bounds and tests. Verified that the test ran without failure for 1000 iterations
> Task :lucene:core:test
:lucene:core:test (SUCCESS): 1000 test(s)
> Task :lucene:core:wipeTaskTemp
The slowest suites (exceeding 1s) during this run:
9.46s TestSloppyMath (:lucene:core)
BUILD SUCCESSFUL in 25s
246 actionable tasks: 175 executed, 71 up-to-date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it is better to have the benchmark. I'm not familiar with what the Rectangle code is doing here: I don't tend to think about sin() being associated with Rectanges...
But the background is that the cos()
and asin()
are needed for this use-case: https://github.com/apache/lucene/blob/main/lucene/core/src/java/org/apache/lucene/document/LatLonPointDistanceComparator.java#L37-L43
We try hard to avoid calling these functions at all, but in some cases such as the sorting, many calls are needed.
Signed-off-by: Ankit Jain <[email protected]>
Signed-off-by: Ankit Jain <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for adding tests and benchies
Description
Wondering if we really need sloppySin anymore. For modern hardware with JDK 17 on recent processors: