Inline coords to boost perf improvements #812
Merged
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
nyurik
force-pushed
the
prep-coordinates
branch
2 times, most recently
from
bae9026
to
6e3ba30
Compare
nyurik
changed the title
rmanoka
requested changes
Apr 18, 2022
| impl<T: CoordNum> From<(T, T)> for Coordinate<T> { | ||
| #[inline] |
There was a problem hiding this comment.
These are good inlines IMO! Thanks!
There was a problem hiding this comment.
Does #[inline] even do much for generic code?
There was a problem hiding this comment.
@lnicola that's a good question - but would require too substantial of a research to pinpoint why adding these inlines boosts performance. I think it is far easier to just add inline everywhere it seems relevant (tiny code, frequent use).
* Inline all simple coordinate methods * use `new()` method in some macros test | improvement ---|--- concave hull f32 | -1.8840% point outside polygon | -4.9574% Polygon Euclidean distance rotating calipers f64 | -1.6313% simplify vwp f32 | -7.0465% the rest were statistically unchanged
nyurik
force-pushed
the
prep-coordinates
branch
from
6e3ba30
to
fa11f57
Compare
2 tasks
nyurik
added a commit
to nyurik/geo
that referenced
this pull request
Apr 21, 2022
This PR includes georust#772 and georust#812 --- This PR focuses on `geo-types` only. This is a part of georust#742. This PR changes the underlying geo-type data structures to support 3D data and measurement values (M and Z values). The PR attempts to cause relatively minor disruptions to the existing users (see breaking changes below). My knowledge of the actual geo algorithms is limited, so please ping me for any specific algo change. ## geo-types restructuring All geo type structs have been renamed from `Foo<T>(...)` to `Foo<T, Z=NoValue, M=NoValue>(...)`, and several type aliases were added: ```rust // old pub struct LineString<T: CoordNum>(pub Vec<Coordinate<T>>); // new pub struct LineString<T: CoordNum, Z: ZCoord=NoValue, M: Measure=NoValue>(pub Vec<Coordinate<T, Z, M>>); pub type LineStringM<T, M=T> = LineString<T, NoValue, M>; pub type LineString3D<T> = LineString<T, T, NoValue>; pub type LineString3DM<T, M=T> = LineString<T, T, M>; ``` ## NoValue magic `NoValue` is an empty struct that behaves just like a number. It supports all math and comparison operations. This means that a `Z` or `M` value can be manipulated without checking if it is actually there. This code works for Z and M being either a number or a NoValue: ```rust pub struct NoValue; impl<T: CoordNum, Z: ZCoord, M: Measure> Sub for Coordinate<T, Z, M> { type Output = Self; fn sub(self, rhs: Self) -> Self { coord! { x: self.x - rhs.x, y: self.y - rhs.y, z: self.z - rhs.z, m: self.m - rhs.m, } } } ``` ## Variant algorithm implementations Function implementations can keep just the original 2D `<T>` variant, or add support for 3D `<Z>` and/or the Measure `<M>`. The above example works for all combinations of objects. This function only works for 2D objects with and without the Measure. ```rust impl<T: CoordNum, M: Measure> Line<T, NoValue, M> { /// Calculate the slope (Δy/Δx). pub fn slope(&self) -> T { self.dy() / self.dx() } } ``` ## Breaking changes * It is no longer possible to create Coordinate with just `x` and `y` values using implicit tuple constructor. Use `coord!` instead.
nyurik
added a commit
to nyurik/geo
that referenced
this pull request
Apr 21, 2022
This PR includes georust#772 and georust#812 --- This PR focuses on `geo-types` only. This is a part of georust#742. This PR changes the underlying geo-type data structures to support 3D data and measurement values (M and Z values). The PR attempts to cause relatively minor disruptions to the existing users (see breaking changes below). My knowledge of the actual geo algorithms is limited, so please ping me for any specific algo change. ## geo-types restructuring All geo type structs have been renamed from `Foo<T>(...)` to `Foo<T, Z=NoValue, M=NoValue>(...)`, and several type aliases were added: ```rust // old pub struct LineString<T: CoordNum>(pub Vec<Coordinate<T>>); // new pub struct LineString<T: CoordNum, Z: ZCoord=NoValue, M: Measure=NoValue>(pub Vec<Coordinate<T, Z, M>>); pub type LineStringM<T, M=T> = LineString<T, NoValue, M>; pub type LineString3D<T> = LineString<T, T, NoValue>; pub type LineString3DM<T, M=T> = LineString<T, T, M>; ``` ## NoValue magic `NoValue` is an empty struct that behaves just like a number. It supports all math and comparison operations. This means that a `Z` or `M` value can be manipulated without checking if it is actually there. This code works for Z and M being either a number or a NoValue: ```rust pub struct NoValue; impl<T: CoordNum, Z: ZCoord, M: Measure> Sub for Coordinate<T, Z, M> { type Output = Self; fn sub(self, rhs: Self) -> Self { coord! { x: self.x - rhs.x, y: self.y - rhs.y, z: self.z - rhs.z, m: self.m - rhs.m, } } } ``` ## Variant algorithm implementations Function implementations can keep just the original 2D `<T>` variant, or add support for 3D `<Z>` and/or the Measure `<M>`. The above example works for all combinations of objects. This function only works for 2D objects with and without the Measure. ```rust impl<T: CoordNum, M: Measure> Line<T, NoValue, M> { /// Calculate the slope (Δy/Δx). pub fn slope(&self) -> T { self.dy() / self.dx() } } ``` ## Breaking changes * It is no longer possible to create Coordinate with just `x` and `y` values using implicit tuple constructor. Use `coord!` instead.
rmanoka
approved these changes
Apr 21, 2022
|
LGTM. Let's give it a couple of days for a few more eyes and merge it if no one has further suggestions |
frewsxcv
approved these changes
Apr 22, 2022
|
bors r+ |
|
Build succeeded: |
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.
new()method in some macrosPerformance improvements
the rest were statistically unchanged
[ ] I added an entry toCHANGES.mdif knowledge of this change could be valuable to users.