Skip to content
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

Introduce faster approximate sinh/atan math functions #49009

Merged
merged 3 commits into from
Nov 14, 2019

Conversation

talevy
Copy link
Contributor

@talevy talevy commented Nov 12, 2019

This commit introduces a new class called ESSloppyMath
that is meant to reflect the purpose of Lucene's SloppyMath,
but add additional unimplemented faster alternatives to math functions.

The two that are used by geotile-grid a lot are sinh/atan. This commit also
switches that implementation to use these new functions.

In a quick elasticsearch rally benchmark for geotile-grid on Switzerland
data points, this shows a (1.22x) 22% speed-up over using Math's functions.

closes #41166.

This commit introduces a new class called ESSloppyMath
that is meant to reflect the purpose of Lucene's SloppyMath,
but add additional unimplemented faster alternatives to math functions.

The two that are used by geotile-grid a lot are sinh/atan.

In a quick elasticsearch rally benchmark for geotile-grid on Switzerland
data points, this shows a (1.22x) 22% speed-up over using Math's functions.

closes elastic#41166.
@talevy talevy added >enhancement :Analytics/Geo Indexing, search aggregations of geo points and shapes labels Nov 12, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (:Analytics/Geo)

Copy link
Contributor

@iverase iverase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I wonder if we should port as well the function for exp and expm1 but that can be done in a follow up PR.

@talevy
Copy link
Contributor Author

talevy commented Nov 13, 2019

thanks Ignacio. I wondered the same about exp and expm1. I decided to delay it since I thought these gains would be good enough and the change would be more minimal. I will follow-up and check to see how things change with the less accurate implementations

@iverase
Copy link
Contributor

iverase commented Nov 13, 2019

Sounds like a good plan to me

Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM2. Except I am not 100% about the header treatment here.

@talevy
Copy link
Contributor Author

talevy commented Nov 14, 2019

@elasticmachine update branch

@talevy talevy merged commit 3ab2de1 into elastic:master Nov 14, 2019
@talevy talevy deleted the essloppymath branch November 14, 2019 20:30
talevy added a commit that referenced this pull request Nov 14, 2019
This commit introduces a new class called ESSloppyMath
that is meant to reflect the purpose of Lucene's SloppyMath,
but add additional unimplemented faster alternatives to math functions.

The two that are used by geotile-grid a lot are sinh/atan.

In a quick elasticsearch rally benchmark for geotile-grid on Switzerland
data points, this shows a (1.22x) 22% speed-up over using Math's functions.

closes #41166.
talevy added a commit to talevy/elasticsearch that referenced this pull request Nov 14, 2019
this resolves incorrectly licensed code in elastic#49009.

ESSloppyMath is removed in favor of preserving as much of the original
FastMath as possible. Since no additional methods are introduced in
ESSloppyMath, this abstraction is removed.
talevy added a commit that referenced this pull request Nov 21, 2019
this resolves incorrectly licensed code in #49009.

ESSloppyMath is made as a wrapper around FastMath.java which is 
not meant to be modified with code beyond the original source
talevy added a commit that referenced this pull request Nov 21, 2019
this resolves incorrectly licensed code in #49009.

ESSloppyMath is made as a wrapper around FastMath.java which is 
not meant to be modified with code beyond the original source
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Geo Indexing, search aggregations of geo points and shapes >enhancement v7.6.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support approximate trigonometric functions in GeoTileUtils
5 participants