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

Halve runtime when simplifying large lines #4

Merged
merged 1 commit into from
May 28, 2022
Merged

Conversation

s3cur3
Copy link
Contributor

@s3cur3 s3cur3 commented May 26, 2022

First, let me say thank you for putting together this library—it saved us a ton of time in our project. 🙏

This PR dramatically improves performance on lines with a large number of points, primarily by reducing the number of times we have to iterate over the list.

  • Removes calls to length/1, since it's linear in the size of the list
  • Cuts down on the number of times we access the last element of a list
  • Cuts down on the number of new lists we construct
  • Cuts down on the number of times we iterate the full list to find the maximum distance

This also adds a very simple benchmark to the test suite, which can be run with:

$ mix test --only bench

Benchmarks on my machines (both running Elixir 13.4 + OTP 24, so the Intel machine has the benefit of the JIT):

Machine Per-iteration Runtime Before After
2019 Intel iMac ~0.15 sec ~0.07 sec
2020 M1 Macbook Air ~0.19 sec ~0.08 sec

This dramatically improves performance on lines with a large number of points, primarily by reducing the number of times we have to iterate over the list.

- Removes calls to `length/1`, since it's linear in the size of the list
- Cuts down on the number of times we access the last element of a list
- Cuts down on the number of new lists we construct
- Cuts down on the number of times we iterate the full list to find the maximum distance

This also adds a very simple benchmark to the test suite, which can be run with:
$ mix test --only bench

On my machine (an M1 Mac running Elixir 13.4 + OTP 24, so no JIT support), the included benchmark runs in roughly 0.19 seconds per iteration in the shipping version of the code. With my changes applied, that drops down to roughly 0.08 seconds.
@spec simplify(%Geo.LineString{}, number) :: %Geo.LineString{}
@spec simplify(Geo.LineString.t(), number) :: Geo.LineString.t()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not perf related, but I couldn't help myself. (Cuts down on unnecessary recompiles by removing the compile-time dependency on the Geo library.)

Copy link
Owner

Choose a reason for hiding this comment

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

👍

@pkinney
Copy link
Owner

pkinney commented May 28, 2022

Thanks, @s3cur3 !

I can definitely confirm a big performance boost (2022 M1 Max Macbook Pro):

Before:

## SimplifyBench
benchmark name                       iterations   average time
short linestring - high tolerance        200000   8.35 µs/op
short linestring - normal tolerance      100000   11.79 µs/op
short linestring - low tolerance         100000   12.00 µs/op

After:

## SimplifyBench
benchmark name                       iterations   average time
short linestring - high tolerance       1000000   1.93 µs/op
short linestring - normal tolerance     1000000   2.68 µs/op
short linestring - low tolerance        1000000   2.85 µs/op

I'll merge this in then release as a 2.0.0 after I do a bit of long-overdue dependency updates and such.

@pkinney pkinney merged commit 3535e1d into pkinney:master May 28, 2022
@pkinney pkinney mentioned this pull request May 28, 2022
@pkinney
Copy link
Owner

pkinney commented May 28, 2022

Released as part of v2.0.0

@s3cur3
Copy link
Contributor Author

s3cur3 commented May 28, 2022

Thank you so much! 🎉

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 this pull request may close these issues.

2 participants