Stack allocate 2d points using tiny_vec. - #222
Conversation
|
Another note, though this greatly speeds up ( I'm not 100%, but I think this is because our parsing is still mostly building up a serde_json::JsonValue per Position, which creates a Vec anyway. I think with these stack allocatable Positions, we'll unlock some good reason to revisit our parsing code. |
dc9a894 to
d1e3dec
Compare
|
As we did before, we could have a look to see who might be relying on Position using GH Code Search and give them a heads up. I would be surprised if there are many explicit uses, but who knows. Given the perf win, I think it's worth the potential downstream inconvenience, especially with the (potential) macro and Index* traits. |
d1e3dec to
9778880
Compare
b8ce67c to
425bf00
Compare
bd9ce7b to
7425d48
Compare
|
After 2.5 years of meditation I have rebased, and I think we should merge this. There will be some breakage for those of our users interacting with Position directly: https://github.com/search?q=%22geojson%3A%3APosition%22+language%3Arust&type=code It seems like most users are using higher levels of abstraction, like the Geometry or Feature api and converting to geo-types or what-have-you. They should be unaffected. In particular I noticed: Happy to hear from any users if they have input on this breaking change in exchange for faster processing. Note I think this is a prerequisite for #223 which unlocks more performance gains. edited: to clarify that I think most users will be unaffected. |
| * BREAKING: Position is now a new type, rather than a type alias for Vec. | ||
| The new type allows for faster handling of GeoJSON in the common (2-D) case by avoid per-coordinate heap allocations. | ||
| ``` | ||
| # BEFORE: Position *was* a Vec |
|
Excellent. As I said in [checks] 2023, this is a great idea. |
7425d48 to
78786aa
Compare
|
@michaelkirk Thank for the ping. This will not be a problem. We can just update the version of geojson crate and fix any incompatabilities on our next release. |
…` with a tiny_vec rather than a Vec. Breaking: Position becomes an opaque struct rather than a (public) type alias.
78786aa to
6ae15c6
Compare
3 participants
CHANGES.mdif knowledge of this change could be valuable to users.Position, which is the fundamental unit for all the various geojson geometries, is currently represented as a (heap allocated)Vec. This PR swaps it out to be aTinyVecwhich can be stack allocated up to a certain number of elements, before moving to the heap.This PR also makes that fact private so we can make similar changes in the future in a non-breaking way.
Unlike #218, this branch maintains the ability to have n-dimensional points, building upon the branch I started in this comment.
This is a breaking change (maybe a substantial one for people relying on the Position type, but shouldn't affect people who are only using the high level APIs.). Switching to tiny_vec will be breaking for these people regardless, and the upside to making the struct opaque like this is that we can now more freely muck about with the internals, e.g. if we one day want to replace tiny_vec, or add a feature flag which allocates 3d stack allocations rather than 2, etc.
We could add a macro like
geojson::position![1.0, 3.0]if people think thePosition::from([1.0, 3.0])is too verbose.What do people think? I've wanted to do something like this for a long time, but kind of avoided it because it's a breaking change, and going to be annoying for anyone who was accessing the Position type directly.
I've tried to alleviate this a bit by implementing
Index/Muton Position. Are there other things I could do to make it break less? If you have a crate that uses geojson, I'd love to have some test cases to integrate this against to see how annoying it is.bench improvements look similar, but slightly less than #218