-
Notifications
You must be signed in to change notification settings - Fork 60
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
WIP: parse points without heap allocation #223
Draft
michaelkirk
wants to merge
28
commits into
mkirk/tiny-vec
Choose a base branch
from
mkirk/parse-with-stack-points
base: mkirk/tiny-vec
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
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
referenced
this pull request
in acteng/overline
Apr 10, 2023
… line-strings, thanks to michaelkirk Unfortunately this increases the parsing time from 11s to 26s
This was referenced Apr 18, 2023
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]).
… custom struct deserialization
…nestly not that much =/
test result: FAILED. 50 passed; 29 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
test result: FAILED. 58 passed; 21 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s
I'm not sure of the mechanics of how this works, but some preliminary measurements don't show a big perf difference. Note the changes to feature_collections.rs are immaterial - only deleting some vestigial code.
test result: FAILED. 63 passed; 16 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.05s
test result: FAILED. 69 passed; 10 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.07s
michaelkirk
force-pushed
the
mkirk/parse-with-stack-points
branch
from
July 13, 2023 21:00
1936b8e
to
bbff616
Compare
michaelkirk
force-pushed
the
mkirk/tiny-vec
branch
from
July 31, 2023 23:09
d1e3dec
to
70ba742
Compare
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.
This is an addendum to #222, I'm not sure it's going to work out, but I wanted to share progress in case someone smarter than me wants to weigh in...
Context: In main today, we represent coordinates (
Positions
) as a Vec of floats. This entails a heap allocation per point, which is slow. #222 changes Positions to be backed by a tiny_vec, which can be stack allocated up to a certain size.However, even with #222, we still use serde_json to parse the input - so those Vec's are created before converting them to the tiny_vec backed representation.
This branch represents my various experiments to avoid allocating Vec's to parse points. I will be the first to admit that it's currently very gnarly. I am not very familiar with the innards of serde (I'm getting a little better through this work!).
Unlike #222, this branch is very far from being mergable. But as a POC I did want to share some modest wins:
Note that this bench is baselined against the changes in #222, not main:
🚨 This WIP currently breaks lots of things - like custom struct serialize/deserialization. If I proceed with this, I'll fix that before marking it "ready for review".