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. |
michaelkirk
force-pushed
the
mkirk/tiny-vec
branch
4 times, most recently
from
dc9a894
to
d1e3dec
Compare
… line-strings, thanks to michaelkirk Unfortunately this increases the parsing time from 11s to 26s
|
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. |
tiny_vec becomes private implementation detail in case we want to change it further in the future. It's admittedly more verbose with this commit. We could clean it up with a pos! macro, or add some helper methods like Value::pt2d([1, 2]).
michaelkirk
force-pushed
the
mkirk/tiny-vec
branch
from
d1e3dec
to
9778880
Compare
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