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

Remove Cow around scalar buffers #720

Merged
merged 5 commits into from
Aug 26, 2024
Merged

Remove Cow around scalar buffers #720

merged 5 commits into from
Aug 26, 2024

Conversation

kylebarron
Copy link
Member

@kylebarron kylebarron commented Aug 26, 2024

Originally scalars were always references onto external buffers. Then in #119 that was changed to try to better support bindings that can't have lifetime references. (E.g. neither python nor JS allow exports with lifetime references, because they can't verify when data will still be valid in those environments).

But over time it turned out that we really needed fully separate structs for these (e.g. OwnedPoint) that don't have their own lifetime parameters. This means that we never actually needed to make Point itself owned. We need to have separate structs anyways for the bindings.

Having Cow on the scalars only serves to make coordinate access slower, because there's an indirection on access to the coordinate buffer.

Change list

  • Remove Cow wrappers on the internal buffers on scalars.

Closes #472

Closes #449

@kylebarron
Copy link
Member Author

kylebarron commented Aug 26, 2024

Running cargo bench --all-features against main shows a decently large performance improvement 🚀

     Running benches/area.rs (target/release/deps/area-f466d9e6b9cb815a)
Gnuplot not found, using plotters backend
area                    time:   [57.263 µs 57.593 µs 58.036 µs]
                        change: [-30.702% -27.953% -23.262%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
  2 (2.00%) high mild
  6 (6.00%) high severe

     Running benches/nybb.rs (target/release/deps/nybb-2ad280365d103699)
Gnuplot not found, using plotters backend
to geo                  time:   [197.23 µs 197.79 µs 198.38 µs]
                        change: [-41.631% -41.206% -40.863%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
  6 (6.00%) high mild
  1 (1.00%) high severe

euclidean distance to scalar point
                        time:   [632.60 µs 635.91 µs 639.94 µs]
                        change: [-15.278% -14.473% -13.715%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
  3 (3.00%) high mild
  6 (6.00%) high severe

     Running benches/translate.rs (target/release/deps/translate-4f4e993ce441eb78)
Gnuplot not found, using plotters backend
translate PolygonArray  time:   [97.402 µs 97.948 µs 98.604 µs]
                        change: [-20.217% -19.746% -19.126%] (p = 0.00 < 0.05)
                        Performance has improved.

@kylebarron kylebarron merged commit cae9d35 into main Aug 26, 2024
22 checks passed
@kylebarron kylebarron deleted the kyle/remove-scalar-cow branch August 26, 2024 20:16
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.

Remove Cow on scalars
1 participant