BTreeMap: move up reference to map's root from NodeRef

[ssomers]

Since the introduction of NodeRef years ago, it also contained a mutable reference to the owner of the root node of the tree (somewhat disguised as *const). Its intent is to be used only when the rest of the NodeRef is no longer needed. Moving this to where it's actually used, thought me 2 things:

• Some sort of "postponed mutable reference" is required in most places that it is/was used, and that's exactly where we also need to store a reference to the length (number of elements) of the tree, for the same reason. The length reference can be a normal reference, because the tree code does not care about tree length (just length per node).
• It's downright obfuscation in from_sorted_iter (transplanted to BTreeMap: better distinguish the root holder from the root node #75329)
• It's one of the reasons for the scary notice on reborrow_mut, the other one being addressed in BTreeMap mutable iterators should not take any reference to visited nodes during iteration #73971.

This does repeat the raw pointer code in a few places, but it could be bundled up with the length reference.

r? @Mark-Simulacrum

[ssomers]

This slightly improves performance, presumably because immutable/owned access (and soon mutable iteration) no longer carries around a useless null pointer:

benchcmp old new --threshold 5
 name                                           old ns/iter  new ns/iter  diff ns/iter   diff %  speedup
 btree::map::find_rand_100                      18           17                     -1   -5.56%   x 1.06
 btree::map::find_seq_100                       18           17                     -1   -5.56%   x 1.06
 btree::map::find_seq_10_000                    40           42                      2    5.00%   x 0.95
 btree::map::first_and_last_0                   32           35                      3    9.38%   x 0.91
 btree::map::first_and_last_100                 46           50                      4    8.70%   x 0.92
 btree::map::insert_rand_100                    42           39                     -3   -7.14%   x 1.08
 btree::map::insert_rand_10_000                 42           39                     -3   -7.14%   x 1.08
 btree::map::insert_seq_100                     50           47                     -3   -6.00%   x 1.06
 btree::map::iter_0                             1,580        1,976                 396   25.06%   x 0.80
 btree::map::iter_1                             2,257        1,817                -440  -19.49%   x 1.24
 btree::map::iteration_1000                     4,114        4,351                 237    5.76%   x 0.95
 btree::map::range_unbounded_unbounded          28,935       36,986              8,051   27.82%   x 0.78
 btree::set::clone_100                          1,908        1,775                -133   -6.97%   x 1.07
 btree::set::clone_100_and_clear                1,904        1,773                -131   -6.88%   x 1.07
 btree::set::clone_100_and_remove_half          2,804        2,663                -141   -5.03%   x 1.05
 btree::set::clone_10k                          202,421      188,552           -13,869   -6.85%   x 1.07
 btree::set::clone_10k_and_clear                203,383      190,601           -12,782   -6.28%   x 1.07
 btree::set::difference_random_100_vs_10k       2,500        2,309                -191   -7.64%   x 1.08
 btree::set::difference_staggered_100_vs_100    728          683                   -45   -6.18%   x 1.07
 btree::set::difference_staggered_100_vs_10k    2,407        2,140                -267  -11.09%   x 1.12
 btree::set::intersection_100_neg_vs_100_pos    26           17                     -9  -34.62%   x 1.53
 btree::set::intersection_100_neg_vs_10k_pos    31           18                    -13  -41.94%   x 1.72
 btree::set::intersection_100_pos_vs_100_neg    26           17                     -9  -34.62%   x 1.53
 btree::set::intersection_100_pos_vs_10k_neg    32           18                    -14  -43.75%   x 1.78
 btree::set::intersection_10k_neg_vs_100_pos    30           18                    -12  -40.00%   x 1.67
 btree::set::intersection_10k_neg_vs_10k_pos    33           19                    -14  -42.42%   x 1.74
 btree::set::intersection_10k_pos_vs_100_neg    30           18                    -12  -40.00%   x 1.67
 btree::set::intersection_10k_pos_vs_10k_neg    33           19                    -14  -42.42%   x 1.74
 btree::set::intersection_random_100_vs_10k     2,355        2,137                -218   -9.26%   x 1.10
 btree::set::intersection_random_10k_vs_100     2,325        2,122                -203   -8.73%   x 1.10
 btree::set::intersection_staggered_100_vs_10k  2,205        1,979                -226  -10.25%   x 1.11
 btree::set::is_subset_100_vs_10k               1,239        1,123                -116   -9.36%   x 1.10

Note that the increase in range_unbounded_unbounded is reproducible and all the weirder because twin sister range_unbounded_vs_iter measures slightly faster than before, but the point of comparing these was that the former should not be much faster than the latter, and that's much more true than before.

[Mark-Simulacrum]

@bors r+

[bors]

📌 Commit ae19245db324e8b46cbc1c0e9b5e778ad6121e51 has been approved by Mark-Simulacrum

[bors]

🌲 The tree is currently closed for pull requests below priority 5, this pull request will be tested once the tree is reopened

[ssomers]

Oh well if you think it's good enough, let me squash those two commits. But I'll be interrogating Miri for this.

[ssomers]

CAD97 estimates that the code still has UB and that my uglifying tweak just happens to sneak past Miri. Though I believe that the same UB exists and has existed for years in master.

[ssomers]

This is what I came up with. Miri happy, me happy.

[ssomers]

It's even better when CI is happy too!

[ssomers]

Continuing from the users.rust-lang.org discussion, I suspect that drain_filter has UB and this PR makes it worse.

In master, remove_kv_tracking sometimes dereferences the root_ptr to pop a level. From then on, nobody should dereference the handles in the iterator. remove_kv_tracking itself returns immediately, but DrainFilter continues to visit elements. The root node gets emptied when the last pair of children gets merged, so there should always be roughly five more elements to iterate when this happens. This (alleged) UB happens whenever drain_filter shrinks a tree to fewer than 11 elements, which includes a lot of test cases that Miri never complains about.

In this PR, this dereferencing to the root happens every iteration, regardless of whether a level needs to be popped. It's no surprise that Miri still keeps quiet about reading through the root, if it didn't complain about writing to the root.

The only correct solution I see is to pop the level at the end of drain_filtering, so in the drop handler. We're navigating through handles, so we mostly don't care that the tree doesn't conform to its invariant yet. But it's still a little awkward.

[ssomers]

Also, the same discussion made me realize that the old system has one trick up its sleeve: by embedding root_ptr in the handle, and only letting it out by a consuming conversion (into_root_mut), it made sure that you couldn't use the handle anymore. But I see some problems with it:

• Nothing and no one explained this, at least to me.
• There was no reason to limit its type to a root, and to call it root. It could have been whatever the owner of the handle wanted to stash away there. Which for immutable and owned handles, could have been nothing at all, at no cost.
• Alas, handles are copied (ptr::read) relatively often, in particular by the double ended iterators, that need a pair of handles anyway. remove_kv_tracking consumes the handle, can invoke into_root_mut and pop a level, but that's no good if there are more handles around. However, DrainFilter is not double ended (and it looks daunting to ever make it), so it doesn't need two handles. There is a ptr::read in DrainFilterInner::next, but it is superfluous. The culprit here is the ptr::read in remove_kv_tracking itself, that specifically serves DrainFilter by keeping track of the gap left behind while the function continues to merge parent nodes. I don't see a way to make this robust.

I tried to restore most of that consume-handles-before-you-dereference-root_ptr safety with a wrapper around the NonNull's, but it's not bullet proof: if you don't consume the handle, the borrow checker will let you use it after dereferencing the root_ptr. But at least you can only deference root_ptr once, and you realize that it was wrong for drain_filter to do it every iteration.

Not squishing because I suspect it's not yet all clean as a whistle.

[ssomers]

And lastly, I thought it was wise to keep the length reference next to the (faked) root reference, because the length reference is real and fully borrow-checked. Now I think this is useless: making sure the length is borrowed correctly does not prove anything about the root reference, and whenever the length needs updating, the root may need updating too. While it is (in theory) possible to update the root without even looking at length. So I joined them into a single pointer to the map.

[ssomers]

Squished everything except some that deserve their own PR

[ssomers]

Put back to draft because #74827 is a better version of the insert refactoring part of this PR. A small (but important) detail here is dereferencing a pointer too early (not picked up by Miri, but still UB I think).

[mark-i-m]

Perhaps mark this as not a draft? What happens if bors tries to merge/close this?

EDIT: lol, I should have read the previous comment... Somebody should bors r- to take it out of the PR queue.

[ssomers]

Uh, I did not notice it's still S-waiting-on-bors. But surely bors would only merge the approved commit from long ago, not all the stuff I piled up since?

Anyway, I'll merge the same change in already, just in case.

[Dylan-DPC-zz]

Yeah I can take a look at this

[ssomers]

@Dylan-DPC No progress is to be expected, I have abandoned research on how to get rid of the S-waiting-on-author label.

@RalfJung It would indeed have been great to know if anyone cares and what it would take to complete this. Apparently it's not the currently first commit, a separation of concerns and a slight performance gain. I've already written about the further commits, the horribly complicated leak amplification. One way to complete that is much more post-processing in DrainFilter's drop handler, but that seems more like procrastination than completion.

[RalfJung]

@ssomers you seem frustrated by the review process here, for which I am sorry. I cannot speak for @Mark-Simulacrum, but for my part I found it hard to understand what your concrete goal was with this PR: when we were just done finalizing the "dormant ref" API, you pushed a lot more changes here that go well beyond the original scope of the PR (the PR title and description do not say anything about leak amplification... or maybe I still misunderstood how this PR relates to leak amplification). That's okay, this happens, but as a reviewer I see this as a signal that you are still working on it. (Though also, from a review perspective, small independent changes are much preferred over big PRs.)

So I was basically waiting for you to say "okay this is good now from my side, I will stop adding more changes to this, please review". The label just reflects this; it usually gets changed when the author posts a comment like that. I am sorry if we did not communicate that properly.

[ssomers]

when we were just done finalizing the "dormant ref" API

Well, "just" is a week, in my head multiplied by older PRs that didn't move either. That is most likely fine for reasonable contributors, but frustrating to me at the moment.

The leak amplification theme came in because Mark spotted the problem with the drop handler I introduced. The 577a229 commit was a quick fix getting rid of the new part of the drop handler and sticking to the title, the later commits fixed the drop handler for good (probably). Since the later commits aren't reviewed at all, are complicated, and I don't think will be further exploited in the near future, I've put back the 577a229 commit.

[ssomers]

Rebased the 577a229 commit without checking much.

@RalfJung I suppose labels work like you write, but not everyone involved here seems to agree - I was suggested to set the label 3 times I remember (like here and here).

[Mark-Simulacrum]

I think this is ready to go. I did want to check -- we resolved the leak amplification in a separate PR, right? I recall flagging an issue with it, but I think it was either not this PR or already resolved. It's a subtle issue so I worry my review may have missed it, as I did previously.

[Dylan-DPC-zz]

Sorry about this, we are working on improving the Contributor Xperience in the rust community so this serves as good feedback for us and we can work on making this better.

A few things to note:
Labels are manual switches and can be changed only by people who have certain permissions on the repo (a github limitation we can't do anything about). The workaround for this is rustbot, which can be used by other members and non-member collaborators to switch labels.
Labels are changed automatically when a reviewer is assigned (since the PR is assumed to be ready for review when a PR is opened (this was in place before draft prs are a thing so the process hasn't changed ) or when there are test failures, pr is approved (r+ only not manual github approvals)
All other changes are done by manually by a team who try and judge the state of a pr and change the labels accordingly.
Requesting a review doesn't change the label (which is i guess what you thought it'll do ) and probably wasn't picked by the team so it didn't do anything other than send a notification to the reviewer that a review is requested but when they checked the PR, they found that it is still waiting on author, so it prolonged.

[RalfJung]

@RalfJung I suppose labels work like you write, but not everyone involved here seems to agree - I was suggested to set the label 3 times I remember (like here and here).

Ah, I wasn't aware some people are suggesting to use the bot for this. It's not the workflow I know but maybe I am old-school already. ;)

FWIW, since you also expressed trouble interacting with the bot, here's the exact invocation you can use to do the label change:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

The bot can be a bit picky about syntax and there's no auto-complete, so getting the command right is tricky... I copied it from the messages bors sends in case of a conflict, which recently were expanded to add that command.

[ssomers]

we resolved the leak amplification in a separate PR, right?

It's not what I understand is meant with "leak amplification", but in the end #75257 fixed the state corruption when using mem::forget on #74762, which is a split-off part of an initial version of this PR. Since then, and in the final version of this PR, the drop handler does nothing more than before, so there cannot be corruption and there is no leak at all.

That said, as Ralf reminded us, the drop handler does exist. It needs to exist to make m.drain_filter(|_, _| true);, without iterating over the drained elements, actually do anything. Obviously if you grab the iterator and mem::forget it, it will not drain any (more) elements (and not corrupt nor leak anything). Therefore, this is semi-official code to efficiently drain the first N elements of a map and any future attempt to introduce post-processing for DrainFilter (like the initial commit here and #74762) is a breaking change. Or, to turn this around, it would be better to implement leak amplification anyway to stop people from exploiting this.

[ssomers]

fixed the state corruption when using mem::forget on #74762

Except that this state corruption was lightweight, and occurred even without involving mem::forget, because I messed up the implementation of the drop handler and it never did any post-processing.

[Mark-Simulacrum]

@bors r+

[bors]

📌 Commit 2b54ab8 has been approved by Mark-Simulacrum

[tmandry]

I've been avoiding these changes in rollups in case there's a perf impact, is there a reason not to worry about that or should they be rollup=never?

[Mark-Simulacrum]

@bors rollup=never

We probably should. They're refactoring that's good even if it is (plausibly) a small perf hit -- benchmarking just BTree usually results in noise, and most of what we've seen so far I think has been just effects of different inlining and such, not much we can do about it.

[bors]

⌛ Testing commit 2b54ab8 with merge ee04f9a...

[bors]

☀️ Test successful - checks-actions, checks-azure
Approved by: Mark-Simulacrum
Pushing ee04f9a to master...