Implement bounded iterators for BTree #20082

pczarn · 2014-12-20T18:56:48Z

Part of collections reform v1, #18424
Also, iteration is simplified:

before
test btree::map::bench::iter_1000                          ... bench:     17177 ns/iter (+/- 6302)
test btree::map::bench::iter_100000                        ... bench:   1735731 ns/iter (+/- 23908)
test btree::map::bench::iter_20                            ... bench:       386 ns/iter (+/- 148)
after
test btree::map::bench::iter_1000                          ... bench:     15777 ns/iter (+/- 346)
test btree::map::bench::iter_100000                        ... bench:   1602604 ns/iter (+/- 73629)
test btree::map::bench::iter_20                            ... bench:       339 ns/iter (+/- 91)

cc @gereeter @cgaebel
r? @gankro

cgaebel · 2014-12-20T19:10:01Z

src/libcollections/btree/node.rs

@@ -1376,7 +1381,7 @@ struct AbsTraversal<Impl> {

 /// A single atomic step in a traversal. Either an element is visited, or an edge is followed
 pub enum TraversalItem<K, V, E> {
-    Elem(K, V),
+    Elem((K, V)),


Just for convenient interop with iterators?

For the obscure opt.map(Elem). Ah, I should add a comment there.

cgaebel · 2014-12-20T19:15:16Z

Can I see tests that look like the iter tests, but bounded? Just make sure they iterate over the same number of elements. I suspect it'd be an interesting comparison. Specifically, I'm looking to see what the cost of creating the iterator looks like.

cgaebel · 2014-12-20T19:17:20Z

src/libcollections/btree/map.rs

+}
+
+/// An endpoint of a range of keys.
+pub enum Bound<T> {


I feel like this shouldn't live in this file. Maybe it should go in libcore? I don't know. It just doesn't seem specific to BTrees.

Agreed. I recommend moving it to the root of libcollections (and std::collections as a re-export).

Gankra · 2014-12-21T00:01:07Z

Quick skim: interesting approach! Excited to dig into this. Sad to see macros. :(

Will review monday if no one else does first.

cgaebel · 2014-12-21T04:55:34Z

Haha "sad to see macros" is pretty much exactly what I thought when scrolling through this.

pczarn · 2014-12-21T12:59:46Z

Should we provide RangedKeys, RangedValues, RangedValuesMut for map, as well as ~~RangedItems~~ RangedKeys for BTreeSet?

interesting approach!

Can you think of some other approach?

Unfortunately, the code is quadrupled for both ends and mut/immutable. Macros deal with the latter duplication.

Gankra · 2014-12-22T15:44:56Z

I was assuming you were going to replace the Two RingBufs with two Vecs, which should better optimize for the "average" case. In fact you've kind've done that. Two stacks and a deque are famously "equivalent", although normally I see that used to implement a deque using two stacks, not the other way around :P

The code complexity benefits probably make it worth it, though.

Gankra · 2014-12-22T15:49:57Z

src/libcollections/btree/map.rs

-                Push(item) => { self.left.push_back(item); },
-                Pop => { self.left.pop_back(); },
+                Push(item) => { self.traversals.push_back(item); },
+                Pop => { self.traversals.pop_back(); },
            }


;_; so elegant

Gankra · 2014-12-30T04:26:54Z

src/libcollections/btree/node.rs

+            self.head_is_edge = true;
+            self.inner.next_kv().map(Elem)
+        })
+    }


Smooth. 💖

Gankra · 2014-12-30T04:37:08Z

Just need to fully parse the macro madness...

Gankra · 2014-12-30T04:43:24Z

src/libcollections/btree/map.rs

+                    loop {
+                        let slice = lca.slice_from(min_key).slice_to(max_key);
+                        match slice.edges {
+                            [ref $($m)* edge] => lca = edge.$as_slices_internal(),


Trying to understand this: basically you make a subslice, and if it contains a single edge you conclude that you must descend deeper? Otherwise (including the case where it's empty?), this must be the LCA?

As always: would love some more docs as to what's going on here.

Gankra · 2015-01-04T03:40:19Z

Basically r=me (feel free to squash), but this is complex enough that I'd appreciate a secondary review from @huonw or @gereeter (or anyone else, really).

Also needs another rebase :(

pczarn · 2015-01-04T23:55:06Z

@gankro, done. The comment "for ranges such as..." is irrelevant for the main example.

pczarn · 2015-01-05T00:22:16Z

src/libcollections/btree/map.rs

+                    // lca represents the Lowest Common Ancestor, above which we never
+                    // walk, since everything else is outside the range to iterate.
+                    //       ___________________
+                    //      |__0_|_80_|_85_|_90_|  (root)


I suppose you wouldn't like to have that irrelevant part. Removed.

emberian · 2015-01-05T06:46:20Z

Needs another rebase :(

pczarn · 2015-01-07T22:37:34Z

@gankro Tested, rebased (in this order 😅 )

Gankra · 2015-01-09T18:44:50Z

@pczarn needs some changes for old_impl_check stuff, I guess?

pczarn · 2015-01-14T15:27:42Z

Updated for old_impl_check

Gankra · 2015-01-19T04:44:48Z

Seems this didn't satisfy all the impl check issues.

Simplify BTree's iterators, too.

pczarn · 2015-01-19T16:49:11Z

@gankro Updated

Gankra · 2015-01-19T16:52:27Z

@bors r+ 429c

Gankra · 2015-01-19T16:53:59Z

Actually this is some pretty hairy stuff.

@bors p=0

?

Gankra · 2015-01-19T16:57:57Z

Aha.

@bors rollup-

bors · 2015-01-19T17:43:47Z

⌛ Testing commit 429c23d with merge 54c9a46...

@gereeter

Part of collections reform v1, #18424 Also, iteration is simplified: ``` before test btree::map::bench::iter_1000 ... bench: 17177 ns/iter (+/- 6302) test btree::map::bench::iter_100000 ... bench: 1735731 ns/iter (+/- 23908) test btree::map::bench::iter_20 ... bench: 386 ns/iter (+/- 148) after test btree::map::bench::iter_1000 ... bench: 15777 ns/iter (+/- 346) test btree::map::bench::iter_100000 ... bench: 1602604 ns/iter (+/- 73629) test btree::map::bench::iter_20 ... bench: 339 ns/iter (+/- 91) ``` cc @gereeter @cgaebel r? @gankro

bors · 2015-01-19T19:40:48Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

rust-highfive assigned Gankra Dec 20, 2014

cgaebel reviewed Dec 20, 2014
View reviewed changes

pczarn force-pushed the btree-bounded-iter branch from b3496c9 to d199ddb Compare December 21, 2014 15:38

Gankra reviewed Dec 22, 2014
View reviewed changes

pczarn force-pushed the btree-bounded-iter branch 4 times, most recently from 4402b6a to d4e6d3d Compare December 23, 2014 22:43

Gankra reviewed Dec 30, 2014
View reviewed changes

src/libcollections/btree/node.rs

self.head_is_edge = true;

self.inner.next_kv().map(Elem)

})

}

Copy link

Contributor

Gankra Dec 30, 2014

Choose a reason for hiding this comment

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

Smooth. 💖

Gankra reviewed Dec 30, 2014
View reviewed changes

Nemo157 mentioned this pull request Jan 3, 2015

Implement Bounded Iterator RoaringBitmap/roaring-rs#5

Closed

pczarn force-pushed the btree-bounded-iter branch 2 times, most recently from 7168297 to 31531c6 Compare January 4, 2015 23:54

pczarn force-pushed the btree-bounded-iter branch from 31531c6 to 1f62517 Compare January 5, 2015 00:20

pczarn reviewed Jan 5, 2015
View reviewed changes

pczarn force-pushed the btree-bounded-iter branch 3 times, most recently from c91326e to 3ee4dc0 Compare January 7, 2015 22:08

pczarn force-pushed the btree-bounded-iter branch from 3ee4dc0 to fe6cca3 Compare January 14, 2015 15:25

pczarn force-pushed the btree-bounded-iter branch from fe6cca3 to cf2a2ab Compare January 19, 2015 16:48

Implement range and range_mut for BTree

429c23d

Simplify BTree's iterators, too.

pczarn force-pushed the btree-bounded-iter branch from cf2a2ab to 429c23d Compare January 19, 2015 16:49

bors merged commit 429c23d into rust-lang:master Jan 19, 2015

pczarn deleted the btree-bounded-iter branch January 24, 2015 09:22

Implement bounded iterators for BTree #20082

Implement bounded iterators for BTree #20082

Uh oh!

Conversation

pczarn commented Dec 20, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgaebel commented Dec 20, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gankra commented Dec 21, 2014

Uh oh!

cgaebel commented Dec 21, 2014

Uh oh!

pczarn commented Dec 21, 2014

Uh oh!

Gankra commented Dec 22, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gankra commented Dec 30, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gankra commented Jan 4, 2015

Uh oh!

pczarn commented Jan 4, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emberian commented Jan 5, 2015

Uh oh!

pczarn commented Jan 7, 2015

Uh oh!

Gankra commented Jan 9, 2015

Uh oh!

pczarn commented Jan 14, 2015

Uh oh!

Gankra commented Jan 19, 2015

Uh oh!

pczarn commented Jan 19, 2015

Uh oh!

Gankra commented Jan 19, 2015

Uh oh!

Gankra commented Jan 19, 2015

Uh oh!

Gankra commented Jan 19, 2015

Uh oh!

bors commented Jan 19, 2015

Uh oh!

bors commented Jan 19, 2015

Uh oh!

Uh oh!