Would a smallvec optimization be useful? #88
Comments
|
For what it's worth, I'm curious why there was no apparent performance gain. My best guess is that when lots of small allocations are made in quick succession you still end up with good cache locality, so the benefits of smallvec are kind of null. |
|
Thanks for sharing this @adeschamps! I guess we'll need to give it a prod with Your changes do highlight something important though: we probably shouldn't use a type alias for |
|
I saw some modest improvements (~5-10%) to serialization/deserialization with my tinyvec branch. And much bigger improvements (78%) with strictly geometry parsing, which I suppose makes sense, since this only affects geometry parsing. bench: `type Postion=TinyVec<[f64; 2]>`Geometry parsingOur benches aren't necessarily representative of real-world-use, but I think this is worth re-investigating. One reason to consider it separately from #130, #131, is that tinyvec (and probably small_vec, I don't really know the difference) is pretty much a drop-in replacement. It would still be a breaking change for anyone relying on the type of Position to be a Vec, but in terms of functionality, people can still parse GeoJSON with 17-Dimensional points or whatever 🙃. I think ultimately a more targeted approach where someone can say "I'm only ever dealing with 2-d geometry, so just use bench: `type Postion = [f64; 2]` vs. tiny_vec |
206: stream reading features r=frewsxcv a=michaelkirk - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- This greatly reduces the peak memory usage needed to parse a GeoJSON FeatureCollection by utilizing the FeatureIterator's trick of seeking to the `"features": [` array without actually parsing the FeatureCollection. Note there are [some robustness issues](#207) with this approach, some of which can be improved, but overall I think it's worth the trade-off. There is a small (0-1%) CPU regression in our deserialization bench due to this change, but the peak memory profile of the `stream_reader_writer` examples decreased 90% - from 2.22MiB to 237Kib. Before: <img width="1624" alt="Screen Shot 2022-09-02 at 2 42 04 PM" src="https://user-images.githubusercontent.com/217057/188239657-ade76677-f3e2-4750-b71d-bed0effbe215.png"> After: <img width="1624" alt="Screen Shot 2022-09-02 at 2 40 29 PM" src="https://user-images.githubusercontent.com/217057/188239645-5274e0ff-71b2-4da6-8ac9-1f72d288c2bb.png"> Notice that overall memory allocations stayed the same (see #88 (comment)), but the peak usage is much lower since we never hold the entire data structure in memory at once. Co-authored-by: Michael Kirk <[email protected]>
michaelkirk
changed the title
Here are the results of a failed experiment.
Currently, the
Positiontype is defined astype Position = Vec<f64>;. Since the majority of instances ofPositionare going to be 2D or 3D points, this seems like a good candidate for aSmallVecoptimization.The memory use (stack, heap, and total) for 2D and 3D points with various storage types are:
So, compared to
Vec, aSmallVecwith a backing store of[f64; 3]is the same size for 2D points, smaller for 3D points, and never uses the heap. If you go past 3 elements, then it heap allocates and there is a constant overhead of 16 bytes per point, but I think that's a rare use case.This seemed like a no-brainer, so I did some benchmarks, and unfortunately it didn't make a noticeable difference. If you're interested, you can check out the
smallvecbranch of my fork. Note that the unit tests don't pass, since it's a breaking API change.In conclusion, it would be a breaking API change for no apparent gain, but since I measured it I figured it's worth sharing.
The text was updated successfully, but these errors were encountered: