-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
BTreeMap navigation done safer & faster #68827
Conversation
ff6df26
to
76635de
Compare
Last commit merely:
|
r? @Amanieu |
I'm really not familiar with the BTree code, but I believe @Mark-Simulacrum has expressed interest. |
I will try to take a look soon, though given the relatively hairy nature of the BTreeMap code it'll likely be some time until I can allocate a big enough block of time to dig in sufficiently. |
Had to move documentation because apparently a recent change in master causes these unused doc comment errors. |
Could you provide a medium-level description of what this is doing? I likely don't have enough time to try to reconstruct that from the diff, which looks to be moving a bunch of stuff around (I guess splitting something into two?). |
It's purely about how the code in navigate.rs is ordered and grouped into functions: more but simpler functions now. The previous code had functions moving from an edge to the next edge while also returning the KV in between, which for mutable trees implies juggling with 2 mutable handles into the same tree, i.e. functions that are unsafe and private and there to support exactly the same code as before #67073. This PR has separate functions taking half steps, from edge to next KV and from KV to bordering edge, sticking to a single handle like the basic navigation in node.rs, i.e. functions that are safe and exposed within btree. There's no intended change to the interface of any previously exposed and used functions, the heart of iterators. There's no actual change to their implementation in my mind, but it's a stupid mind and benchmarks contradict it. The "we have to do this last" code that #58431 repaired still happens last and Miri is still happy. |
3d488fa
to
ba16bbc
Compare
Last commit is to rename the new functions and squash. Meanwhile my mind remembers that there is one "functional" change: the new function called |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good; left a few comments that I think will simplify things even more.
Ok(internal_kv) => return Ok(internal_kv.forget_node_type()), | ||
Err(last_edge) => match last_edge.into_node().ascend() { | ||
Ok(parent_edge) => parent_edge, | ||
Err(root) => return Err(root.forget_type()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we avoid duplicating this code? It looks like we should be able to, by starting out with let mut parent_edge = self;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, indeed. Not quite starting from there but from:
let mut edge = self.forget_node_type();
which has to be defined for edges too, but then it works. Benchmarking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seriously? Another factor 2 boost? It's getting ridiculous.
unwrap_unchecked(next_level) | ||
macro_rules! def_next_kv { | ||
{ pub fn $name:ident : $adjacent_kv:ident } => { | ||
/// Given a leaf edge handle, returns [`Result::Ok`] with a handle to the KV next to it, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Given a leaf edge handle, returns [`Result::Ok`] with a handle to the KV next to it, | |
/// Given a leaf edge handle, returns [`Result::Ok`] with a handle to the neighboring KV (either left or right, see name of this function), |
I'm not really happy about having to say things like this but I was pretty confused otherwise. It may make sense to lift the documentation out of the macro, if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if it's possible anymore. I had to move it there because of these unused doc comment errors.
But if the simplification above pays off, it may be worth while to inline the macro and repeat the code, like I did for next_leaf_edge
and next_back_leaf_edge
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. We may be able to also use generics instead of the macro, though that wouldn't solve the documentation issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can probably also pass identifiers/tokens to the macro and let it generate the right documentation each time. But when I do "x.py doc", the doc for that internal function appears nowhere. And who would look for it anyways.
*self = next_edge; | ||
let kv = ptr::read(self).next_kv(); | ||
let kv = unwrap_unchecked(kv.ok()); | ||
*self = ptr::read(&kv).next_leaf_edge(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also replace the *self = ...
with a ptr::write(self, ...)
because that way it'll do the right thing even if self gains a drop impl or so.
ba16bbc
to
d68df4b
Compare
Latest benchmarks:
Note that without an [inline] on the new |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
…p_first, pop_last
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace the merge commit with its parent(s) (i.e., we do not want merge commits, but e.g. a rebase or so is fine). Otherwise, this looks good, I will approve once that's done.
e45e33d
to
d035095
Compare
d035095
to
9f7b58f
Compare
Rebasing would make things a lot easier, but kills benchmarking. So here's one with the actual commits squashed again, and the commit merged in master cherry picked, still based on the last master that allowed benchmarking. If that's still not okay, I'll rebase. |
Yeah, that works too. Just didn't want the merge commit :) I'm surprised/amazed/confused by the benchmark deltas, but since they're not negative they seem fine. Thanks! @bors r+ |
📌 Commit 9f7b58f has been approved by |
☀️ Test successful - checks-azure |
It turns out that there was a faster way to do the tree navigation code bundled in #67073, by moving from edge to KV and from KV to next edge separately. It extracts most of the code as safe functions, and contains the duplication of handles within the short wrapper functions.
This somehow hits a sweet spot in the compiler because it reports boosts all over the board:
The
first_and_last
ones are noise (they don't do iteration), the others seem genuine.As always, approved by Miri.
Also, a separate commit with some more benchmarks of mutable behaviour (which also benefit).
r? @cuviper