-
Notifications
You must be signed in to change notification settings - Fork 68
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
Avoiding SmallVec
in SelectionIterator
#163
Comments
It was worse when this was a But I think if we want to provide an I did try implementing internal iteration once in #37 but the recursive function overhead was not so nice. I could try to resurrect that code if you interested in trying that. It does obviously not provide an |
That's really interesting, thank you! I shouldn't need Internal iteration with recursion could work well and the benchmarks in that PR seem promising. I would be a little concerned with the extra call stack overhead depending on the tree depth, but the tradeoff might be worth it here. I'm currently using Maybe there might be some hybrid approach where we track parent nodes per tree level and the current child's offset on the stack (like the existing buffer), which would avoid frequently moving nodes in and out of the buffer. I'm not sure how something like that would compare to recursion though. |
I pushed the envelope queries as a follow-up commit onto the branch/PR. |
There's a bit of noise in the profile I'm using, but on average it looks like improves the performance for my use case (~185ms with internal iteration vs. ~234ms with |
- [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 `rstar/CHANGELOG.md` if knowledge of this change could be valuable to users. --- This avoids the overhead of allocating an internal buffer to keep track of upcoming nodes when implementing the `Iterator` trait. I also found a mistake in the old code from #37 (lack of early return in the parent case) and now the benchmarks also look somewhat nicer, i.e. directly comparing internal and external iteration on the same data set: ```console locate_at_point (successful) time: [115.62 ns 116.51 ns 117.43 ns] locate_at_point_int (successful) time: [66.831 ns 67.264 ns 67.653 ns] locate_at_point (unsuccessful) time: [167.70 ns 168.03 ns 168.34 ns] locate_at_point_int (unsuccessful) time: [167.90 ns 168.28 ns 168.64 ns] ``` Closes #163
In
SelectionIterator
andSelectionIteratorMut
we use aSmallVec
to queue nodes as we're iterating over the nodes in the tree, e.g.:rstar/rstar/src/algorithm/iterators.rs
Line 46 in 84d1265
Constantly resizing this
SmallVec
seems to show up on some of my profiles where I have relatively dense trees.I was wondering if there might be a way to avoid
SmallVec
here somehow but it's not clear how it could work. Maybe some other crates already have found a good pattern to avoid this kind of iteration queue? e.g., if we could somehow represent a flattened position of the next node then maybe that could work, but I don't know if this would be possible without changing how nodes are stored.The text was updated successfully, but these errors were encountered: