Simplify NodeHeader by avoiding slices in BTreeMaps with shared roots

[ssomers]

Simplify a complicated piece of code that creates slices of keys in node leaves.

[rust-highfive]

r? @kennytm

(rust_highfive has picked a reviewer for you, use r? to override)

[ssomers]

r? @RalfJung

[ssomers]

The simplification stems from the observation that slices on immutable trees are almost exclusively used to get_unchecked a particular item. The only exception happens through the (locally) public keys function used only genuinely by search_linear. It really does a linear search through a slice of keys in leaf nodes, which is empty if the leaf node is the shared root. One might expect map iterators to also iterate over the items of a leaf through a slice, but they access individual keys instead.

This might cure current & future bugs and enable change because it's easier to understand, and it might cause future bugs and hinders changes because of a somewhat sneaky precondition "shared roots not welcome here" introduced in the (very locally) public function keys.

Tests fine on 32-bit & 64-bit Windows, Linux & Miri (with debug assertions). Performance change (without debug assertions :^)

cargo benchcmp old2.txt new2.txt --threshold 5
 name                                           old2.txt ns/iter  new2.txt ns/iter  diff ns/iter   diff %  speedup
 btree::map::find_rand_100                      17                18                           1    5.88%   x 0.94
 btree::map::find_seq_100                       17                18                           1    5.88%   x 0.94
 btree::map::find_seq_10_000                    40                43                           3    7.50%   x 0.93
 btree::map::insert_seq_10_000                  94                99                           5    5.32%   x 0.95
 btree::map::iter_20                            40                42                           2    5.00%   x 0.95
 btree::set::difference_random_100_vs_100       738               690                        -48   -6.50%   x 1.07
 btree::set::difference_staggered_100_vs_100    765               721                        -44   -5.75%   x 1.06
 btree::set::intersection_random_100_vs_100     669               617                        -52   -7.77%   x 1.08
 btree::set::intersection_staggered_100_vs_100  716               640                        -76  -10.61%   x 1.12
 btree::set::intersection_staggered_10k_vs_10k  68,719            57,891                 -10,828  -15.76%   x 1.19
 btree::set::is_subset_100_vs_100               451               426                        -25   -5.54%   x 1.06
 btree::set::is_subset_10k_vs_10k               44,665            40,234                  -4,431   -9.92%   x 1.11

In my experience, this means there is no difference. The optimizer just chooses to focus on some other test cases.

One alternative is to take this a step further: don't make slices at all, and don't sneak in a precondition, but require a very visible index argument as implemented in this branch. This also - avoids "leaking" the slice concept outside node.rs (so for instance, storing a key-value pair as a pair would be local to the file).

[RalfJung]

Regarding your last comment at #67459 (comment):

There's something still not quite right: into_key_slice_mut checks alignment and size in the same way as into_key_slice does, but then doesn't also carefully cast to the NodeHeader type it checked on. Instead into_key_slice_mut bluntly invokes as_leaf_mut, which itself comments "We are mutable, so we cannot be the shared root".
[...]
But also, since Miri confirms that doing this is fine, doesn't that mean that the effort in into_key_slice to access as NodeHeader, and the whole existence of keys_start, is superfluous already?

I don't follow, didn't you say that into_key_slice_mut is never actually called for the shared root? That would fully explain why we don't need the special handling there.

[RalfJung]

Maybe add a doc comment saying that it is up to the caller to ensure that this is not the shared root?

[ssomers]

For an internal function, isn't the assert obvious enough?

[RalfJung]

It's still a big file. But I don't feel very strongly about this one.

[Mark-Simulacrum]

I would like to see this function made unsafe and a comment on it that it is unsafe to call with a shared root.

Alternatively, I think it is plausible that we can keep it safe and make the assert non-debug (realistically the one constant-comparison seems super unlikely to be the performance problem in real world code), when we're dealing with a BTreeMap that's comparing right and left anyway.

[RalfJung]

I'm a big fan of this, but I don't know BtreeMap well enough to review this, I am afraid. Can someone from @rust-lang/libs please take over?

[RalfJung]

Also Cc @Gankra

[RalfJung]

Well, that's what this comment tricked me into believing, but you (perhaps unintentionally) pointed out that get_mut on a fresh map does come there, so I added that at the start of test_basic_small. It won't happen anymore if we include this PR, of course.

Oh I see. Is that the range_search diff?

[ssomers]

Well, that's what this comment tricked me into believing,

Turns out that this comment (on as_leaf_mut) "We are mutable, so we cannot be the shared root" was factually right: we're never the shared root. It's just that "We are mutable" is not a good reason. I got confused because get_mut can be invoked on a shared root, and eventually invokes this. But it searches its key using the non-mutable keys variant, and only when it finds an index for the key (which implies the tree wasn't empty), then it visits as_leaf_mut.

[ssomers]

I moved everything that doesn't directly relate to the keys_start proposal into #67725.

[rust-highfive]

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-12-30T12:27:03.9850601Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-30T12:27:03.9866578Z ##[command]git config gc.auto 0
2019-12-30T12:27:03.9871612Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-30T12:27:03.9876519Z ##[command]git config --get-all http.proxy
2019-12-30T12:27:03.9882510Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67686/merge:refs/remotes/pull/67686/merge
---
2019-12-30T12:32:31.6718325Z     Checking backtrace v0.3.40
2019-12-30T12:32:31.7593053Z error[E0107]: wrong number of type arguments: expected 2, found 3
2019-12-30T12:32:31.7593498Z    --> src/liballoc/collections/btree/node.rs:535:46
2019-12-30T12:32:31.7593745Z     |
2019-12-30T12:32:31.7594093Z 535 |         if (mem::align_of::<NodeHeader<K, V, K>>() > mem::align_of::<NodeHeader<K, V>>()
2019-12-30T12:32:31.7599073Z 
2019-12-30T12:32:31.7619798Z error[E0107]: wrong number of type arguments: expected 2, found 3
2019-12-30T12:32:31.7620170Z    --> src/liballoc/collections/btree/node.rs:536:48
2019-12-30T12:32:31.7620409Z     |
2019-12-30T12:32:31.7620409Z     |
2019-12-30T12:32:31.7620776Z 536 |             || mem::size_of::<NodeHeader<K, V, K>>() != mem::size_of::<NodeHeader<K, V>>())
2019-12-30T12:32:31.7621182Z 
2019-12-30T12:32:32.3157566Z error: aborting due to 2 previous errors
2019-12-30T12:32:32.3157966Z 
2019-12-30T12:32:32.3159124Z For more information about this error, try `rustc --explain E0107`.
---
2019-12-30T12:32:32.3365773Z   local time: Mon Dec 30 12:32:32 UTC 2019
2019-12-30T12:32:32.5990703Z   network time: Mon, 30 Dec 2019 12:32:32 GMT
2019-12-30T12:32:32.5998374Z == end clock drift check ==
2019-12-30T12:32:39.4825986Z 
2019-12-30T12:32:39.4953460Z ##[error]Bash exited with code '1'.
2019-12-30T12:32:39.4992546Z ##[section]Starting: Checkout
2019-12-30T12:32:39.4994350Z ==============================================================================
2019-12-30T12:32:39.4994410Z Task         : Get sources
2019-12-30T12:32:39.4994459Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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 @TimNN. (Feature Requests)

[RalfJung]

r? @dtolnay matching #67725

[Mark-Simulacrum]

I would like to see this function made unsafe and a comment on it that it is unsafe to call with a shared root.

Alternatively, I think it is plausible that we can keep it safe and make the assert non-debug (realistically the one constant-comparison seems super unlikely to be the performance problem in real world code), when we're dealing with a BTreeMap that's comparing right and left anyway.

[Mark-Simulacrum]

I would like to see the functions that cannot be called on the shared root made unsafe (yes, it seems like that probably means everything ends up being unsafe, but that's fine IMO) -- better than pretending it's safe.

Other than that, I believe this PR is ready to land.

[ssomers]

I would like to see the functions that cannot be called on the shared root made unsafe

Including functions like push_front (where I added the debug_assert because it ends up in as_leaf_mut), all the way up to the BTreeMap code that actually makes sure there's no shared root, right? That sure seems a significant change set. Wouldn't you rather see that in a separate PR, up front or later?

[Mark-Simulacrum]

Ah, I meant specifically the ones we've changed in this PR (into_key_slice was the one specifically that I meant). But yes, the rest can be done in a separate PR.

[RalfJung]

I am torn here... I agree in principle they should be unsafe, but some of these function are non-trivial in size, and without rust-lang/rfcs#2585 we'll lose all syntactic indication of which operations in them need extra scrutiny.

[Mark-Simulacrum]

Yeah, I agree it's not quite clear that it'll be a win. I would at least like to see us make sure that the functions are properly annotated with the preconditions that must hold before we do much more refactoring on the APIs, as currently it's sort of "dig into the function and figure it out" each time.

[ssomers]

There's actually not much lost, to my surprise, when I do the same changes in the master preceding this PR (not fully tested yet).

It still seems an unfair treatment with regards to debug_assert on other properties, like for instance pop_level assuming that there is a level to pop.

[Mark-Simulacrum]

It still seems an unfair treatment with regards to debug_assert on other properties, like for instance pop_level assuming that there is a level to pop.

We would want to apply the same treatment there, too. But it's not immediately clear based on the diff exactly what you mean in terms not losing much -- I would need to dig in further. Overall though I think the most important thing in this case (for private code) is not the unsafe annotation so much as documenting preconditions.

[ssomers]

I meant that most of the functions in node.rs that become unsafe, didn't have any subtle distinction between unsafe blocks and safe parts. That code is mostly in map.rs, and there this change only adds (even) more unsafe blocks.

[ssomers]

I think the most important thing in this case (for private code) is not the unsafe annotation so much as documenting preconditions.

Those are there now, both informally (comment line) and very formally (debug_assert).

[Mark-Simulacrum]

@bors r+

I would like to spend some time myself digging into the code to try and ensure we've documented all the preconditions and so forth, but that need not block this PR, which I believe is in a sufficiently good state to merge. Let's move forward.

[bors]

📌 Commit 3e947ef has been approved by Mark-Simulacrum

[bors]

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