Skip to content
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
wants to merge 28 commits into
base: mkirk/tiny-vec
Choose a base branch
from

Conversation

michaelkirk
Copy link
Member

@michaelkirk michaelkirk commented Apr 8, 2023

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:

$ cargo bench --bench="*" -- --baseline="tiny-vec"
 
parse (countries.geojson)
                        time:   [1.8220 ms 1.8453 ms 1.8728 ms]
                        change: [-2.6785% -1.9295% -1.1463%] (p = 0.00 < 0.05)
                        Performance has improved.

FeatureReader::features (countries.geojson)
                        time:   [5.2148 ms 5.2285 ms 5.2429 ms]
                        change: [-10.682% -10.259% -9.8336%] (p = 0.00 < 0.05)
                        Performance has improved.

# 👀 this input is small, so maybe not very representative, but still -
# I'm curious why the wins are so huge here while so relatively modest elsewhere.
# I'd guess that in the other benches, much of the remaining time is spent parsing floats.
parse (geometry_collection.geojson)
                        time:   [489.36 ns 490.73 ns 492.07 ns]
                        change: [-96.840% -96.814% -96.794%] (p = 0.00 < 0.05)
                        Performance has improved.

     Running benches/serialize.rs (target/release/deps/serialize-52a1ff0867a899e5)
serialize geojson::FeatureCollection struct (countries.geojson)
                        time:   [2.8810 ms 2.8919 ms 2.9034 ms]
                        change: [+1.2959% +1.7045% +2.1246%] (p = 0.00 < 0.05)
                        Performance has regressed.

     Running benches/to_geo_types.rs (target/release/deps/to_geo_types-9c7e7ca989cc32d0)
quick_collection        time:   [66.437 µs 66.623 µs 66.819 µs]
                        change: [-0.1052% +0.4353% +0.9697%] (p = 0.11 > 0.05)
                        No change in performance detected.

🚨 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".

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
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]).
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant