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

Avoiding SmallVec in SelectionIterator #163

Closed
grovesNL opened this issue Apr 24, 2024 · 5 comments · Fixed by #164
Closed

Avoiding SmallVec in SelectionIterator #163

grovesNL opened this issue Apr 24, 2024 · 5 comments · Fixed by #164

Comments

@grovesNL
Copy link
Contributor

In SelectionIterator and SelectionIteratorMut we use a SmallVec to queue nodes as we're iterating over the nodes in the tree, e.g.:

current_nodes: SmallVec<[&'a RTreeNode<T>; 24]>,

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.

@adamreichold
Copy link
Member

It was worse when this was a Vec instead of SmallVec. ;-)

But I think if we want to provide an Iterator-based, we do need the buffer as rstar uses a "dense" representation of the tree without padding out the nodes to a fixed size (so that sizes could just be computed).

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 Iterator implementation though, so I am not sure if this works for you.

@adamreichold
Copy link
Member

@grovesNL Please give #164 a try if it works for you. (Let me know if you need another query method besides the four examples provided over there.)

@grovesNL
Copy link
Contributor Author

That's really interesting, thank you! I shouldn't need Iterator for my use case.

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 locate_in_envelope_intersecting but it looks like the selection functions are mostly private, so I might have to modify a copy of your branch before I can try it out.

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.

@adamreichold
Copy link
Member

I pushed the envelope queries as a follow-up commit onto the branch/PR.

@grovesNL
Copy link
Contributor Author

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 Iterator, averaged over three runs each).

@dabreegster dabreegster mentioned this issue Apr 28, 2024
2 tasks
github-merge-queue bot pushed a commit that referenced this issue Nov 3, 2024
- [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
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 a pull request may close this issue.

2 participants