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

Add intrusive iterators to BTree. #19796

Closed
wants to merge 1 commit into from

Conversation

cgaebel
Copy link
Contributor

@cgaebel cgaebel commented Dec 13, 2014

Unfortunately, tree structures are intrinsically slower to
iterate over externally than internally. This can be
demonstrated in benchmarks. In fact, it's so bad at external
iteration that calling .find on each element in succession
is currently slightly faster.

This patch implements a faster intrusive way to iterate over
BTrees. This is about 5x faster, but you lose all iterator
composition infrastructure. This is a tradeoff that is
acceptable in some applications.

Relevant benchmarks:

test btree::map::bench::intrusive_iter_1000                ... bench:      2658 ns/iter (+/- 602)
test btree::map::bench::intrusive_iter_100000              ... bench:    346353 ns/iter (+/- 189565)
test btree::map::bench::intrusive_iter_20                  ... bench:        55 ns/iter (+/- 16)
test btree::map::bench::iter_1000                          ... bench:     15892 ns/iter (+/- 3717)
test btree::map::bench::iter_100000                        ... bench:   1383714 ns/iter (+/- 444706)
test btree::map::bench::iter_20                            ... bench:       366 ns/iter (+/- 104)

r? @gankro @huonw @gereeter

@aturon how does this fit into 1.0 stabilization plans. Is
marking this as #[experimental] enough?

Unfortunately, tree structures are intrinsically slower to
iterate over externally than internally. This can be
demonstrated in benchmarks. In fact, it's so bad at external
iteration that calling `.find` on each element in succession
is currently slightly faster.

This patch implements a faster intrusive way to iterate over
BTrees. This is about 5x faster, but you lose all iterator
composition infrastructure. This is a tradeoff that is
acceptable in some applications.

Relevant benchmarks:

```
test btree::map::bench::intrusive_iter_1000                ... bench:      2658 ns/iter (+/- 602)
test btree::map::bench::intrusive_iter_100000              ... bench:    346353 ns/iter (+/- 189565)
test btree::map::bench::intrusive_iter_20                  ... bench:        55 ns/iter (+/- 16)
test btree::map::bench::iter_1000                          ... bench:     15892 ns/iter (+/- 3717)
test btree::map::bench::iter_100000                        ... bench:   1383714 ns/iter (+/- 444706)
test btree::map::bench::iter_20                            ... bench:       366 ns/iter (+/- 104)
```

r? @gankro @huonw

@aturon how does this fit into 1.0 stabilization plans. Is
marking this as #[experimental] enough?
@cgaebel cgaebel force-pushed the btree-intrusive-iter branch from 605c97b to def348d Compare December 13, 2014 01:17
}
}

intrusive_into_iter_impl(self.root, &mut f);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you believe that the recursive version will be faster/more efficient than an explicit stack?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do, but only because the benchmarks as written agree.

@Gankra
Copy link
Contributor

Gankra commented Dec 14, 2014

Just like to say that this is really unfortunate to have to provide for performance. 😞

@bluss
Copy link
Member

bluss commented Dec 15, 2014

What's the B value and how do the results compare with a bit larger fan out?

My immediate thought was to micro optimizations, like replacing Zip with an iterator that knows to do the bounds check once, but I realize now that the Zip part is common to both alternatives in this comparison. Is this a hint that a simpler b-tree external iterator can be written (maybe without range support?)

@Gankra
Copy link
Contributor

Gankra commented Dec 15, 2014

I believe @pczarn was investigating this.

One "easy" one would be to not support DoubleEnded.

Another is we could probably ditch the RingBufs for Vecs, since pop_front should only happen if you're mixing next and next_back, which should already be slow. Also they can physically only get to like max ~40 depth or something.

@Gankra
Copy link
Contributor

Gankra commented Dec 22, 2014

I'd like to flesh these ideas out out-of-tree for now. In particular https://github.com/reem/rust-traverse and https://github.com/Gankro/collect-rs/ are exploring this space right now.

Only a performance issue, not a back-compat hazard.

@Gankra Gankra closed this Dec 22, 2014
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.

3 participants