deprecate direct field access for geo-types #818
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
michaelkirk
force-pushed
the
mkirk/private-fields
branch
15 times, most recently
from
4ef17c5 to
fedb660
Compare
plus some other methods to help migrate from direct field access. This is intended to be a non-breaking change for now, to give people an upgrade window. In an upcoming *breaking* release of geo-types we'll drop pub field access altogether. This is in pursuit of adding support for 3D/4D geometries. We'll leverage generics in a way that is intended to avoid runtime cost for our mostly 2D user base. See #5 for more. This commit includes a bunch of new methods that correspond to the deprecated field access. See geo-types/CHANGES.md for a summary. `#[inline]` hints were added to maintain performance (it's actually improved in some places!) geo was updated to address all the deprecations.
michaelkirk
force-pushed
the
mkirk/private-fields
branch
from
fedb660 to
2d2f973
Compare
This was referenced Apr 23, 2022
|
Pending some conversation in #816, I'm going to close this for now, lest someone spend a bunch of time reviewing. (Sorry if you've already started). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CHANGES.mdif knowledge of this change could be valuable to users.FIXES #816
deprecate direct field access for geo-types, plus some other methods to help migrate from direct field access.
This is intended to be a non-breaking change for now, to give people an upgrade window. In an upcoming breaking release of geo-types we'll drop pub field access altogether.
This is in pursuit of adding support for 3D/4D geometries. When we do that, we'll leverage generics in a way that is intended to avoid runtime cost for our mostly 2D user base. See #5 for more.
This commit includes a bunch of new methods that correspond to the deprecated
field access. See geo-types/CHANGES.md for a summary.
#[inline]hints were added to maintain performance (it's actually improved insome places!)
geo was updated to address all the deprecations.
review
This PR is really big. I think the changes to really scrutinize are in geo-types. Applying it all to
geois large but very rote.I think there's potential to add further methods to make code that uses geo-types a little nicer, but I tried to strike the right balance between minimizing this diff while not committing egregiously worse code that would immediately churn in a followup pr.
docs
A lot of the work was getting the docs right. You can build them yourself, or check them here: doc.tar.gz
(We should have a CI action that uploads a web accessible preview of the docs... I'll create an issue)
perf
Cargo bench is mostly unchanged, but surprisingly got a bit better for a couple things - in particular:
I was curious why this would be. It seems like most of the difference is attributable to inlining the
Rect::newconstructor. Which is pretty wild, because we're not actually givingRect::newany additional inline hints. It's plausible the the new inline hints on Coord are having percolating effects.I don't have a great way to share that analysis, but here's a screenshot showing the "bottom up" view of rstar::bulk_load while profiling the simplify benches:
Before, on the left, within bulk_load we were spending about 25% of the time in Rect::new. After, it goes away completely.
Heres the entire criterion output if you want a closer look:
criterion.tar.gz
output of `cargo bench --bench "*" -- --baseline main`
$ git rev-parse HEAD 2d2f9734995a59569099d9b4af861f0bbae4e6f4 $ cargo bench --bench "*" -- --baseline main Compiling geo-types v0.7.4 (/Users/mkirk/src/georust/geo/geo-types) Compiling wkt v0.10.0 Compiling geo v0.20.1 (/Users/mkirk/src/georust/geo/geo) Compiling geo-test-fixtures v0.1.0 (/Users/mkirk/src/georust/geo/geo-test-fixtures) Compiling jts-test-runner v0.1.0 (/Users/mkirk/src/georust/geo/jts-test-runner) Finished bench [optimized] target(s) in 13.28s Running unittests (target/release/deps/area-855e28849021a92f) Gnuplot not found, using plotters backend area time: [8.2317 us 8.2349 us 8.2387 us] change: [-1.8445% -1.6177% -1.3895%] (p = 0.00 < 0.05) Performance has improved. Found 4 outliers among 100 measurements (4.00%) 3 (3.00%) high mild 1 (1.00%) high severe Running unittests (target/release/deps/concave_hull-3fc5ee84b9d2bfc5) Gnuplot not found, using plotters backend concave hull f32 time: [3.9138 ms 3.9151 ms 3.9165 ms] change: [-1.3098% -1.1612% -1.0223%] (p = 0.00 < 0.05) Performance has improved. Found 1 outliers among 100 measurements (1.00%) 1 (1.00%) high severe concave hull f64 time: [4.4853 ms 4.4885 ms 4.4918 ms] change: [-0.8010% -0.5925% -0.3983%] (p = 0.00 < 0.05) Change within noise threshold. Found 1 outliers among 100 measurements (1.00%) 1 (1.00%) high mild Running unittests (target/release/deps/contains-63588a9ec8c3ed8b) Gnuplot not found, using plotters backend point in polygon time: [29.806 ns 29.814 ns 29.823 ns] change: [+0.3900% +0.6508% +0.9131%] (p = 0.00 < 0.05) Change within noise threshold. Found 16 outliers among 100 measurements (16.00%) 3 (3.00%) high mild 13 (13.00%) high severe point outside polygon time: [5.3372 ns 5.3443 ns 5.3516 ns] change: [-1.1952% -0.8311% -0.4798%] (p = 0.00 < 0.05) Change within noise threshold. Found 4 outliers among 100 measurements (4.00%) 4 (4.00%) high mild Running unittests (target/release/deps/convex_hull-6cb335bd5faaf6fd) Gnuplot not found, using plotters backend convex hull f32 time: [238.18 us 238.30 us 238.49 us] change: [-0.2670% -0.1761% -0.0908%] (p = 0.00 < 0.05) Change within noise threshold. Found 7 outliers among 100 measurements (7.00%) 4 (4.00%) high mild 3 (3.00%) high severe convex hull f64 time: [239.44 us 239.49 us 239.53 us] change: [-0.6070% -0.2911% +0.1400%] (p = 0.15 > 0.05) No change in performance detected. Found 10 outliers among 100 measurements (10.00%) 4 (4.00%) high mild 6 (6.00%) high severe convex hull with collinear random i64 time: [48.715 ms 48.758 ms 48.801 ms] change: [-0.1923% -0.0309% +0.1341%] (p = 0.72 > 0.05) No change in performance detected. Found 2 outliers among 100 measurements (2.00%) 1 (1.00%) low mild 1 (1.00%) high mild Running unittests (target/release/deps/euclidean_distance-bc6fac8c303d54fc) Gnuplot not found, using plotters backend Polygon Euclidean distance RTree f64 time: [27.112 us 27.119 us 27.125 us] change: [-0.6660% -0.4632% -0.2686%] (p = 0.00 < 0.05) Change within noise threshold. Found 5 outliers among 100 measurements (5.00%) 2 (2.00%) high mild 3 (3.00%) high severe Polygon Euclidean distance rotating calipers f64 time: [19.989 us 20.001 us 20.016 us] change: [-0.2129% +0.0192% +0.2731%] (p = 0.88 > 0.05) No change in performance detected. Found 6 outliers among 100 measurements (6.00%) 4 (4.00%) high mild 2 (2.00%) high severe Running unittests (target/release/deps/extremes-8415e0a254d01d86) Gnuplot not found, using plotters backend extremes f32 time: [16.921 us 16.924 us 16.928 us] change: [-0.8815% -0.6828% -0.5003%] (p = 0.00 < 0.05) Change within noise threshold. Found 5 outliers among 100 measurements (5.00%) 1 (1.00%) low mild 2 (2.00%) high mild 2 (2.00%) high severe extremes f64 time: [16.886 us 16.890 us 16.894 us] change: [-0.4735% -0.3434% -0.2306%] (p = 0.00 < 0.05) Change within noise threshold. Found 4 outliers among 100 measurements (4.00%) 4 (4.00%) high severe Running unittests (target/release/deps/frechet_distance-d39f7fa200ab99dd) Gnuplot not found, using plotters backend Benchmarking frechet distance f32: Warming up for 3.0000 s Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 8.3s, enable flat sampling, or reduce sample count to 50. frechet distance f32 time: [1.6431 ms 1.6432 ms 1.6435 ms] change: [+0.3392% +0.4194% +0.4921%] (p = 0.00 < 0.05) Change within noise threshold. Found 8 outliers among 100 measurements (8.00%) 7 (7.00%) high mild 1 (1.00%) high severe Benchmarking frechet distance f64: Warming up for 3.0000 s Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 8.5s, enable flat sampling, or reduce sample count to 50. frechet distance f64 time: [1.6918 ms 1.6920 ms 1.6923 ms] change: [+0.4849% +0.5517% +0.6211%] (p = 0.00 < 0.05) Change within noise threshold. Found 5 outliers among 100 measurements (5.00%) 2 (2.00%) high mild 3 (3.00%) high severe Running unittests (target/release/deps/geodesic_distance-585ac551a71c3f42) Gnuplot not found, using plotters backend geodesic distance f64 time: [440.72 ns 441.70 ns 442.74 ns] change: [-0.8843% -0.5960% -0.2980%] (p = 0.00 < 0.05) Change within noise threshold. Found 17 outliers among 100 measurements (17.00%) 6 (6.00%) high mild 11 (11.00%) high severe Running unittests (target/release/deps/relate-4e43449cddce0e54) Gnuplot not found, using plotters backend relate overlapping 50-point polygons time: [30.491 us 30.497 us 30.504 us] change: [-0.3225% -0.2631% -0.1989%] (p = 0.00 < 0.05) Change within noise threshold. Found 10 outliers among 100 measurements (10.00%) 3 (3.00%) low mild 3 (3.00%) high mild 4 (4.00%) high severe Running unittests (target/release/deps/rotate-782ee575c44a9478) Gnuplot not found, using plotters backend rotate f32 time: [42.581 us 42.586 us 42.591 us] change: [-0.0331% +0.0427% +0.1046%] (p = 0.24 > 0.05) No change in performance detected. Found 12 outliers among 100 measurements (12.00%) 5 (5.00%) high mild 7 (7.00%) high severe rotate f64 time: [48.764 us 48.774 us 48.785 us] change: [+0.1467% +0.2790% +0.4650%] (p = 0.00 < 0.05) Change within noise threshold. Found 4 outliers among 100 measurements (4.00%) 1 (1.00%) high mild 3 (3.00%) high severe Running unittests (target/release/deps/simplify-407a9cdfb29cfbcc) Gnuplot not found, using plotters backend simplify simple f32 time: [68.521 us 68.664 us 68.807 us] change: [-0.6969% -0.4201% -0.1588%] (p = 0.00 < 0.05) Change within noise threshold. simplify simple f64 time: [86.558 us 86.588 us 86.616 us] change: [-0.3586% -0.2222% -0.0898%] (p = 0.00 < 0.05) Change within noise threshold. Found 11 outliers among 100 measurements (11.00%) 7 (7.00%) low mild 1 (1.00%) high mild 3 (3.00%) high severe Running unittests (target/release/deps/simplifyvw-2d178e8147bc38c5) Gnuplot not found, using plotters backend simplify vw simple f32 time: [182.08 us 182.61 us 183.15 us] change: [-1.0460% -0.8368% -0.6350%] (p = 0.00 < 0.05) Change within noise threshold. Found 11 outliers among 100 measurements (11.00%) 11 (11.00%) high mild simplify vw simple f64 time: [195.62 us 196.35 us 197.20 us] change: [-4.9151% -4.3032% -3.7086%] (p = 0.00 < 0.05) Performance has improved. Found 9 outliers among 100 measurements (9.00%) 9 (9.00%) high mild Benchmarking simplify vwp f32: Warming up for 3.0000 s Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 5.9s, enable flat sampling, or reduce sample count to 60. simplify vwp f32 time: [1.1719 ms 1.1726 ms 1.1735 ms] change: [-13.607% -13.525% -13.447%] (p = 0.00 < 0.05) Performance has improved. Found 2 outliers among 100 measurements (2.00%) 1 (1.00%) high mild 1 (1.00%) high severe Benchmarking simplify vwp f64: Warming up for 3.0000 s Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 5.9s, enable flat sampling, or reduce sample count to 60. simplify vwp f64 time: [1.1721 ms 1.1734 ms 1.1746 ms] change: [-2.8011% -2.6356% -2.4896%] (p = 0.00 < 0.05) Performance has improved. Found 1 outliers among 100 measurements (1.00%) 1 (1.00%) high mild Running unittests (target/release/deps/vincenty_distance-719460e9a4e37b64) Gnuplot not found, using plotters backend vincenty distance f32 time: [146.94 ns 146.96 ns 146.98 ns] change: [+0.0507% +0.1035% +0.1589%] (p = 0.00 < 0.05) Change within noise threshold. Found 8 outliers among 100 measurements (8.00%) 4 (4.00%) high mild 4 (4.00%) high severe vincenty distance f64 time: [253.71 ns 253.77 ns 253.83 ns] change: [+0.0571% +0.1358% +0.2119%] (p = 0.00 < 0.05) Change within noise threshold. Found 8 outliers among 100 measurements (8.00%) 4 (4.00%) high mild 4 (4.00%) high severe