BTreeMap mutable iterators should not take any reference to visited nodes during iteration

[ssomers]

Fixes #73915, overlapping mutable references during BTreeMap iteration

r? @RalfJung

[ssomers]

benchcmp old newer --threshold 5

 name                                   old ns/iter  newer ns/iter  diff ns/iter   diff %  speedup
 btree::map::find_rand_100              17           18                        1    5.88%   x 0.94
 btree::map::find_seq_10_000            39           43                        4   10.26%   x 0.91
 btree::map::insert_rand_100            41           36                       -5  -12.20%   x 1.14
 btree::map::insert_rand_10_000         41           36                       -5  -12.20%   x 1.14
 btree::map::insert_seq_100             49           45                       -4   -8.16%   x 1.09
 btree::map::insert_seq_10_000          94           102                       8    8.51%   x 0.92
 btree::map::iter_0                     1,724        1,509                  -215  -12.47%   x 1.14
 btree::map::iter_100                   2,706        3,294                   588   21.73%   x 0.82
 btree::map::iteration_mut_1000         3,916        4,215                   299    7.64%   x 0.93
 btree::map::iteration_mut_100000       458,680      487,070              28,390    6.19%   x 0.94
 btree::map::range_included_excluded    390,220      410,035              19,815    5.08%   x 0.95
 btree::map::range_included_included    434,763      397,835             -36,928   -8.49%   x 1.09
 btree::map::range_unbounded_unbounded  28,255       36,724                8,469   29.97%   x 0.77
 btree::map::range_unbounded_vs_iter    28,810       34,102                5,292   18.37%   x 0.84
 btree::set::is_subset_100_vs_10k       1,269        1,394                   125    9.85%   x 0.91

This compares the best over 10 runs each. Still not particularly clear, but not quite hopeful. Using get_unchecked actually makes it seem worse.

[ssomers]

Your "only way to do this currently" works too, and says basically no change in performance: benchcmp old newest --threshold 5

 name                                           old ns/iter  newest ns/iter  diff ns/iter   diff %  speedup
 btree::map::find_rand_100                      17           18                         1    5.88%   x 0.94
 btree::map::first_and_last_100                 47           42                        -5  -10.64%   x 1.12
 btree::map::first_and_last_10k                 64           74                        10   15.62%   x 0.86
 btree::map::insert_rand_100                    41           36                        -5  -12.20%   x 1.14
 btree::map::insert_rand_10_000                 41           36                        -5  -12.20%   x 1.14
 btree::map::insert_seq_100                     49           45                        -4   -8.16%   x 1.09
 btree::map::insert_seq_10_000                  94           99                         5    5.32%   x 0.95
 btree::map::range_included_excluded            390,220      410,650               20,430    5.24%   x 0.95
 btree::map::range_unbounded_unbounded          28,255       37,597                 9,342   33.06%   x 0.75
 btree::set::difference_random_100_vs_10k       2,521        2,745                    224    8.89%   x 0.92
 btree::set::intersection_random_100_vs_10k     2,322        2,581                    259   11.15%   x 0.90
 btree::set::intersection_random_10k_vs_100     2,305        2,649                    344   14.92%   x 0.87
 btree::set::intersection_staggered_100_vs_10k  2,125        2,233                    108    5.08%   x 0.95

Thanks to baby steps, I think I understand that code better than anything before, but I think you'll agree it's not great to have (more of) this sort of code written out in btree directly. But I couldn't even write a generic function reusable for keys and vals.

[RalfJung]

Btw, what are "alies" (in the PR title)? Probably you meant "allies" but even then I don't get it.^^

[RalfJung]

Thanks to baby steps, I think I understand that code better than anything before, but I think you'll agree it's not great to have (more of) this sort of code written out in btree directly. But I couldn't even write a generic function reusable for keys and vals.

Indeed, what we should have is #60639 so you can just index the raw slice directly. #73987 makes that hard though.

[ssomers]

Good question, who are these allies? I was thinking of values_mut, range_mut passes through the mutable next_unchecked too. So does remove_kv_tracking but it discards the result, so I think that's all.

Which takes us back to your comment "Doing the descend [...] invalidates the references returned by into_kv_mut" now residing in that mutable next_unchecked. I wonder if it's still relevant; in fact, I wonder if that ever was enough of a solution? If the result of into_kv_mut is wrecked by navigating the tree, how can you navigate the tree while the caller can cling onto the result of into_kv_mut? Except that there was no test case clinging on to the result. Back in #58431 you wrote that len was the culprit already, and now that len is trouble free. I tried reverting that special case and Miri says it's fine now.

[ssomers]

Sigh... I just checked if "Miri is too slow" to check 144 references, i.e. 3 levels. It is not. It is, in fact, very fast to point out that something is still broken.

It's not due to the last "Try disturbing the past" commit. It happens if test_values_mut operates on 19 or more elements.

I have no idea how to figure out what Miri is upset about. The 19th element, for inserts with increasing values and if I'm not mistaken, causes the 2nd child node to split, leaving two child nodes with 6 elements each, the last child node with 5 elements, and the root 2 elements.

[ssomers]

About the original code:

• MaybeUninit::get_mut(&mut (*leaf).vals[idx]) is the code without raw pointers that keeps Miri happy on the new test (commit 5500b31b28e81b6e8f46ae19d4c50a8d2be10b86)
• MaybeUninit::get_mut(&mut (((*leaf).vals)[idx])) works like the above (I'm not sure it's the same though)
• ((*leaf).vals).get_unchecked_mut(idx).get_mut() compiles & tests fine but fails under Miri (didn't manage to compile with an explicit MaybeUninit::get_mut)
• ((*leaf).vals).get_mut(idx).unwrap().get_mut() fails like the above

[ssomers]

The way I understand the issue now, it seems very likely this would fail for any other mutable iterator implementation that doesn't apply social distancing to its elements, so everything but linked lists. It's definitely not hard to make Miri mad at VecDeque::iter_mut.

[RalfJung]

Except that there was no test case clinging on to the result. Back in #58431 you wrote that len was the culprit already, and now that len is trouble free. I tried reverting that special case and Miri says it's fine now.

Which special case? Fixing len just falls out of making as_leaf return a raw pointer instead of a reference, doesn't it?

MaybeUninit::get_mut(&mut (*leaf).vals[idx]) is the code without raw pointers that keeps Miri happy on the new test (commit 5500b31)

The crucial part is that it is without references: the only reference being created here is to (*leaf).vals[idx], which does not overlap with the other elements. That's why this is okay from an aliasing perspective.

&mut (((*leaf).vals)[idx])

This is the same.

The other two create a reference to the entire vals array, which overlaps with the mutable reference that was handed out earlier. This I would indeed expect them to fail.

It's definitely not hard to make Miri mad at VecDeque::iter_mut.

Uh-oh.^^ I was going to apply that mutable-iterator testcase to other collections, didn't expect issues to show up so quickly though. Do you want to open an issue for VecDeque?

[ssomers]

Which special case?

The one I thought I had reverted by commit aa0681fef26beb812884de5f0a92b6f5aab4a006... but looking closer, this doesn't revert the order you set in in #58431, but merely stores the result of navigation later and obfuscates the important order. When I try to really swap the order, Miri complains again (on the 12 element test that's currently committed).

[ssomers]

open an issue for VecDeque?

Sure, you'll soon get #74029 on your plate.

[RalfJung]

So is MIN_INSERTS_HEIGHT_2 still broken or did you latest changes fix that?

[ssomers]

It's still broken from 19 elements on. I only changed the code backing immutable iterators.

[RalfJung]

The original mutable iterator testcase failed for 2 elements, so something presumably also changed there?

Do you know what is the problem with 19 elements or do you want me to take a look?

[ssomers]

so something presumably also changed there?

Well, what changed is most of this PR? You handled len and other code using as_leaf, into_key_val_mut is also completely using raw pointers, but it's still not enough for 19 or more elements (*). This PR also tweaks the API used by immutable iterators to keep it in line with the API for mutable iterators.

No, I have no idea what the problem is and I see you write it's quite awful to debug so definitely have a look.

(*) The threshold is 19 elements if inserted in ascending order, like the test case does. In addition, one can insert up to 5 more smaller (negative) keys (which should land in the first child node).

[ssomers]

I think I got there through reasoning: 19 elements is indeed the first tree with children and two elements at the root and the first tree where next_kv ascends back into the root, while there is an outstanding reference inside the root already. I bet it's actually forget_node_type that "refers to" the root node again.

[bors]

☔ The latest upstream changes (presumably #76217) made this pull request unmergeable. Please resolve the merge conflicts.

[Mark-Simulacrum]

Rebased and reviewed locally -- this seems good to me. I didn't re-run tests or miri tests, though -- will wait on PR CI to pass, but r=me with that done.

[RalfJung]

Looks like this PR does not add the Miri testcase from #73915, but I can do that in a follow-up PR (after verifying that it indeed works in Miri).

[RalfJung]

Oh never mind, this removes the #[cfg_attr(miri, ignore)], so Miri will pick it up. (Though currently there's another test suite failure so I'll have to specifically check locally.)

I'll submit a test based on test_all_refs. I thought one of @ssomers's PRs added that but I cannot find that right now?

[Mark-Simulacrum]

@bors r+

[bors]

📌 Commit 8158d56 has been approved by Mark-Simulacrum

[bors]

⌛ Testing commit 8158d56 with merge d92155b...

[bors]

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

[ssomers]

I'll submit a test based on test_all_refs. I thought one of @ssomers's PRs added that

test_values_mut is still there, since #74666, and now Miri knows about it.

[RalfJung]

Oh, those tests already landed, great. :)

[Mark-Simulacrum]

This looks like it was a small performance improvement on token-stream-stress-check of around 1.2%. Not really expected but nice anyway :)