Implement append for b-trees.
#32466
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
| type Item = (K, V); | ||
|
|
||
| fn next(&mut self) -> Option<(K, V)> { | ||
| let res = match (&self.left_cur, &self.right_cur) { |
There was a problem hiding this comment.
Is it possible to use this instead (i.e. without explicitly getting a reference to the fields)?
match (self.left_cur, self.right_cur) {
(Some((ref left_key, _)), Some((ref right_key, _))) => {
...EDIT: Never mind, I expect not.
|
r? @apasel422 |
| } | ||
| } | ||
| }, | ||
| Internal(_) => unreachable!(), |
There was a problem hiding this comment.
This unreachable branch seems a bit ugly to me. It would be better if cur_node just explicitly stored a Leaf node. The loop for descending to a leaf node would just need to use a separate variable for its internal state.
|
This looks excellent on the correctness and algorithmic fronts. The code could use some mostly cosmetic restructuring and possibly some optimization (though that could wait to a later PR), but otherwise everything seems good to me. Thank you, @jooert! |
|
@alexcrichton Does this need to be gated? |
|
Ah yeah let's add all new methods as |
|
Thanks for all your comments! I just pushed a new commit that should fix all of the issues mentioned. I'm not satisifed with the |
|
This looks good to me, pending a final review by @gereeter. |
| @@ -1809,58 +1778,39 @@ fn handle_underfull_node<'a, K, V>(node: NodeRef<marker::Mut<'a>, | |||
| Merged(handle.merge().into_node()) | |||
| } else { | |||
| if is_left { | |||
| Stole(handle.steal_left().into_node()) | |||
| handle.steal_left(); | |||
| Stole(handle.into_node()) | |||
There was a problem hiding this comment.
This return is the same, regardless of whether is_left is true or not, so it can be moved out of the if statement.
|
@jooert What's the status of this? |
|
☔ The latest upstream changes (presumably #33049) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Sorry for the long delay, I resolved the merge conflicts and fixed the remaining issues. |
|
@gereeter Want to give this one last look? |
| if right_child.len() < node::CAPACITY / 2 { | ||
| // We need to steal. | ||
| let num_steals = node::CAPACITY/2 - right_child.len(); | ||
| match right_child.ascend() { |
There was a problem hiding this comment.
Instead of descending to right_child, ascending back to parent (which is the same thing as internal.last_edge()), and then descending again, it might be better to calculate right_child.len() with a reborrow, then do the final descend unconditionally after possibly stealing:
let right_child_len = internal.reborrow().last_edge().descend().len();
if right_child_len < node::CAPACITY / 2 {
// ...
}
cur_node = internal.last_edge().descend();I think this might also help LLVM generate slightly better code, as it doesn't understand that .descend().ascend() does nothing.
|
Sorry for the delay in responding. Aside from those two small nits, this looks good to me. |
The algorithm implemented here is linear in the size of the two b-trees. It firsts creates a `MergeIter` from the two b-trees and then builds a new b-tree by pushing key-value pairs from the `MergeIter` into nodes at the right heights. Three functions for stealing have been added to the implementation of `Handle` as well as a getter for the height of a `NodeRef`. The docs have been updated with performance information about `BTreeMap::append` and the remark about B has been removed now that it is the same for all instances of `BTreeMap`.
|
@gereeter Thanks for your patience and for reviewing all the changes! I hope this is now ready to roll. |
|
After the final changes and squashing, LGTM. |
Implement `append` for b-trees. I have finally found time to revive #26227, this time only with an `append` implementation. The algorithm implemented here is linear in the size of the two b-trees. It firsts creates a `MergeIter` from the two b-trees and then builds a new b-tree by pushing key-value pairs from the `MergeIter` into nodes at the right heights. Three functions for stealing have been added to the implementation of `Handle` as well as a getter for the height of a `NodeRef`. The docs have been updated with performance information about `BTreeMap::append` and the remark about B has been removed now that it is the same for all instances of `BTreeMap`. cc @gereeter @gankro @apasel422
I have finally found time to revive #26227, this time only with an
appendimplementation.The algorithm implemented here is linear in the size of the two b-trees. It firsts creates
a
MergeIterfrom the two b-trees and then builds a new b-tree by pushingkey-value pairs from the
MergeIterinto nodes at the right heights.Three functions for stealing have been added to the implementation of
Handleaswell as a getter for the height of a
NodeRef.The docs have been updated with performance information about
BTreeMap::appendandthe remark about B has been removed now that it is the same for all instances of
BTreeMap.cc @gereeter @gankro @apasel422