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

GeometrySnapper runtime dominated by slow hypot(). #1110

Closed
micycle1 opened this issue Jan 10, 2025 · 4 comments · Fixed by #1112
Closed

GeometrySnapper runtime dominated by slow hypot(). #1110

micycle1 opened this issue Jan 10, 2025 · 4 comments · Fixed by #1112

Comments

@micycle1
Copy link
Contributor

micycle1 commented Jan 10, 2025

PR #923 replaced replaced Math.sqrt(x^2+y^2) calls with Math.hypot(x,y).

However this is a huge problem for things like the GeometrySnapper that call Coordinate.distance() repeatedly.

For instance, Geometry snapper is spending 83% of its runtime in hypot() (on goem with 4k points), which is unacceptable.

image

@dr-jts
Copy link
Contributor

dr-jts commented Jan 10, 2025

Do you have a comparison with using Math.sqrt(dx*dx + dy * dy)? That's required as a baseline to see the actual increase in time for hypot.

I'm fine with reverting the hypot change, either everywhere or in performance-critical places. But it would be good to have metrics that confirm the issue.

@micycle1
Copy link
Contributor Author

micycle1 commented Jan 10, 2025

Just pulled the code, and swapped out the hypot call.
Performance is roughly double.
Bit hard to capture exactly in profiling since it's so call-heavy and instrumentation distorts it (was profiling before).

@dr-jts
Copy link
Contributor

dr-jts commented Jan 11, 2025

Good enough for me. I will look into reverting that PR.

@dr-jts
Copy link
Contributor

dr-jts commented Jan 11, 2025

It's not surprising that Math.hypot is so much slower - it's doing a lot more work (see code here).

I am thinking about creating a MathUtil.hypot function which is a wrapper over Math.sqrt(x^2+y^2), to modularize the code and make it more self-documenting. This could be used everywhere, or unwrapped in performance-critical areas (although I'm guessing that the JIT will make the performance pretty good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants