Simplify into_key_slice_mut #67725

ssomers · 2019-12-30T11:23:05Z

Remove a rare and tiny but superfluous run-time check from into_key_slice_mut.

In #67459, I wrote that "get_mut [...] does visit into_key_slice_mut" and that was wrong. No function that operates on a map that (still) has a shared root ever dives into into_key_slice_mut. So it's more clear to remove the (previously existing, and always incomplete) code it has for dealing with shared roots, as well as a petty performance improvement for those using exotically aligned key types.

~~Also, some testing of the range function initially added to #67686 but hardly related.~~

r? @RalfJung

src/liballoc/collections/btree/node.rs

Dylan-DPC-zz · 2019-12-30T11:43:44Z

src/liballoc/collections/btree/node.rs

-                )
-            }
+        debug_assert!(!self.is_shared_root());
+        // We cannot be the shared root, so `as_leaf_mut` is okay.


We? I'd prefer something more objective like the slice or whatever it is

I didn't actually write that comment, and think it's somewhat superfluous, but please come up with a concrete suggestion here, like:

// self cannot be the shared root

// This NodeRef cannot be the shared root

// Our node reference cannot be the shared root

// This node cannot be the shared root

I would go with:

self cannot be the shared root

I don't think that's any better, on the contrary, it comes across as poor grammar and capitalization to me. Also, there are already at least 3 comments of this "we = self" style in the file, so at least one author and one reviewer didn't object.

The style of the public API documentation of Option, Vector, BTreeSet, leads to (in unscientifically determined order of popularity):

// The node cannot be the shared root

// The NodeRef cannot be the shared root

If there are multiple instances involved, we explicitly use self (with backticks). Somehow, its awkward capitalization is carefully avoided, except Vec::split_off

RalfJung · 2019-12-30T18:45:41Z

Please don't assign these PRs to me, I already told you I am not familiar enough with this code to review any actual changes to the behavior, like adding new assumptions.

@rust-lang/libs @Gankra could someone please take over?

src/liballoc/collections/btree/node.rs

ssomers · 2019-12-31T07:35:25Z

Please don't assign these PRs to me

Sorry, I thought I'd skim off the part you had already discussed. I don't know whether I should make more or fewer PRs.

Dylan-DPC-zz · 2019-12-31T07:37:44Z

@ssomers don't worry, i'll handle the reassignment.

r? @dtolnay

ssomers · 2019-12-31T08:02:13Z

like adding new assumptions

Note that this PR doesn't really add assumptions, it makes them explicit as debug-assertions. They were already somewhat documented and we've figured out that violating them would fail, for some types of key. And that the current BTreeMap code offers no way to fail them.

RalfJung · 2019-12-31T10:53:02Z

I don't know whether I should make more or fewer PRs.

Generally, smaller PRs are easier to review, so splitting them up is fine. I was just complaining about your choice of reviewer. ;)

Note that this PR doesn't really add assumptions, it makes them explicit as debug-assertions. They were already somewhat documented and we've figured out that violating them would fail, for some types of key. And that the current BTreeMap code offers no way to fail them.

Fair -- to some extend this just makes explicit what must already have been true.

src/liballoc/collections/btree/node.rs

ssomers · 2019-12-31T14:01:56Z

Latest commit: squashed, with fewer asserts and more comments, and without the range testing.

ssomers · 2020-01-01T12:21:09Z

A new year, and a new insight. The old comment we're about to spread here "that this is indeed a proper leaf node, and not the shared root." is confusing. In much of node.rs, and in the literature on B-trees, nodes are either leaf or internal (sometimes called non-leaf). In some places, like as_leaf, an internal node is-a leaf node in the object-oriented sense, sharing the same data structure (but adding edges). If "proper leaf node" means anything, it excludes internal nodes, and that is not what is meant or happens here.

So unless someone has a better idea, I'm going to back this PR down to into_key_slice_mut only and start a new PR with a comment Valhalla.

ssomers · 2020-01-04T01:03:10Z

A nail in the coffin (better late than never): when you change get_mut to search in the slice served by the current master's into_key_slice_mut (instead of the slice served by into_key_slice now), everything seems to work, no debug assertions go off, but Miri is indeed as unhappy as it once was with into_key_slice.

…ruppe Tweak and extend internal BTreeMap documentation, including debug asserts. Gathered from work on various other pull requests (e.g. rust-lang#67725, rust-lang#67686).

RalfJung · 2020-01-06T09:06:31Z

A nail in the coffin (better late than never)

Good catch!
So, what would be the best place to put a comment in the source explaining this?

ssomers · 2020-01-06T09:44:36Z

I don't understand. It's not really a catch, just an obvious (in hindsight) proof of what was assumed/feared. Why would you explain that the implementation doesn't work reliably? Either fix it (copy and adapt the else-part of into_key_slice), or, as in this PR, simplify worries away.

RalfJung · 2020-01-06T10:26:12Z

Wait, I thought you had to patch things to cause Miri to fail? My understanding of what you said is that:

Current master is correct, because get_mut uses into_key_slice.
When changing get_mut to use into_key_slice_mut, the error this introduces is caught by our current test suite with Miri.

Is that not what you said?

ssomers · 2020-01-06T10:59:31Z

Yes, that's what I mean. Except I would argue that the current master is correct but booby-trapped against future developments.

Or in other words, the public API is fine but the internal API upon which BTreeMap is built has a bug.

RalfJung · 2020-01-06T13:58:31Z

Right, so you confirmed that this PR does the right thing. :)

However, what I meant above is that there should be a comment in get_mut explaining why it doesn't use the "mut" operation.

ssomers · 2020-01-06T19:25:56Z

Oh, now I understand. I'll look into it (when my PC works again), but it's not like get_mut avoids anything in particular. Most likely remove too could have been conceived to search in keys_mut. It's just so that tree searches serve multiple callers, and support all kinds of tree mutability, and searching itself does not mutate, so it's simplest for tree searches to stay immutable all the way and leave the transition to mutable over to the caller (through the returned index).

RalfJung · 2020-01-06T19:56:16Z

Okay, so it's more complicated. But is there some reasonable place to explain that all these searches must be done in the immutable version to avoid calling into_key_slice_mut on the shared root?

ssomers · 2020-01-06T21:43:45Z

I'll mention in the search functions that, even though they're defined generically over BorrowType, they operate as if they have an immutable map on their hands.

src/liballoc/collections/btree/search.rs

src/liballoc/collections/btree/node.rs

src/liballoc/collections/btree/search.rs

Co-Authored-By: Ralf Jung <post@ralfj.de>

RalfJung · 2020-01-09T11:06:44Z

Great, thanks for enduring! At this point we went over this often enough, and you gathered enough evidence, and also nobody else felt confident enough to review BTreeMap stuff, that I think I can reasonably r+ this. Let's hope this is the right call...

r? @RalfJung @bors r+

bors · 2020-01-09T11:06:45Z

📌 Commit 9b92bf8 has been approved by RalfJung

ssomers · 2020-01-09T11:09:32Z

Another thing I don't understand is that GitHub batch thing. It seems the 2nd attempt took it now, but we have 2 genuine commits. Doesn't that need to be squashed?

RalfJung · 2020-01-09T11:11:00Z

What "GitHub batch thing"?

ssomers · 2020-01-09T11:14:29Z

What "GitHub batch thing"?

Just a git user interface extension of GitHub, to accept multiple review suggestions in batch. It's why the commit title is "Apply suggestions from code review". But it's still a separate commit and I thought bors wants a single commit per PR.

RalfJung · 2020-01-09T11:17:16Z

Ah that.

No, bors doesn't want a single commit per PR. What we don't want is merge commits in the PR. We also prefer a clean history, but frequently "clean" means "one commit per logical change", not just squashing everything into one commit. The two commits here seem fine to me, though if you want to squash them I won't stop you.

@RalfJung

Simplify into_key_slice_mut Remove a rare and tiny but superfluous run-time check from into_key_slice_mut. In rust-lang#67459, I wrote that "`get_mut` [...] does visit `into_key_slice_mut`" and that was wrong. No function that operates on a map that (still) has a shared root ever dives into `into_key_slice_mut`. So it's more clear to remove the (previously existing, and always incomplete) code it has for dealing with shared roots, as well as a petty performance improvement for those using exotically aligned key types. ~~Also, some testing of the `range` function initially added to rust-lang#67686 but hardly related.~~ r? @RalfJung

@ghost

Rollup of 10 pull requests Successful merges: - #66254 (Make Layout::new const) - #67122 (Do not deduplicate diagnostics in UI tests) - #67358 (Add HashSet::get_or_insert_owned) - #67725 (Simplify into_key_slice_mut) - #67935 (Relax the Sized bounds on Pin::map_unchecked(_mut)) - #67967 (Delay bug to prevent ICE in MIR borrowck) - #67975 (Export public scalar statics in wasm) - #68006 (Recognise riscv64 in compiletest) - #68040 (Cleanup) - #68054 (doc: add Null-unchecked version section to mut pointer as_mut method) Failed merges: - #67258 (Introduce `X..`, `..X`, and `..=X` range patterns) r? @ghost

rust-highfive assigned RalfJung Dec 30, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 30, 2019

Dylan-DPC-zz suggested changes Dec 30, 2019

View reviewed changes

ssomers force-pushed the into_key_slice_mut branch from 73a4044 to 2cb6d52 Compare December 30, 2019 12:06

ssomers mentioned this pull request Dec 30, 2019

Simplify NodeHeader by avoiding slices in BTreeMaps with shared roots #67686

Merged

RalfJung reviewed Dec 30, 2019

View reviewed changes

src/liballoc/collections/btree/node.rs Outdated Show resolved Hide resolved

RalfJung reviewed Dec 30, 2019

View reviewed changes

src/liballoc/collections/btree/node.rs Outdated Show resolved Hide resolved

rust-highfive assigned dtolnay and unassigned RalfJung Dec 31, 2019

RalfJung reviewed Dec 31, 2019

View reviewed changes

src/liballoc/collections/btree/node.rs Outdated Show resolved Hide resolved

ssomers force-pushed the into_key_slice_mut branch from f5c33a1 to e2b6d43 Compare December 31, 2019 14:00

ssomers changed the title ~~Add debug-asserts, comments, tests, and simplify into_key_slice_mut~~ Simplify into_key_slice_mut and add debug-asserts & comments Dec 31, 2019

ssomers force-pushed the into_key_slice_mut branch from e2b6d43 to aedf8ca Compare January 2, 2020 16:20

ssomers changed the title ~~Simplify into_key_slice_mut and add debug-asserts & comments~~ Simplify into_key_slice_mut Jan 2, 2020

ssomers mentioned this pull request Jan 2, 2020

Tweak and extend internal BTreeMap documentation, including debug asserts. #67812

Merged

ssomers force-pushed the into_key_slice_mut branch from aedf8ca to 9f47064 Compare January 7, 2020 15:21

RalfJung reviewed Jan 7, 2020

View reviewed changes

src/liballoc/collections/btree/search.rs Outdated Show resolved Hide resolved

Simplify into_key_slice_mut and document bits and bobs

37b5cca

ssomers force-pushed the into_key_slice_mut branch from 9f47064 to 37b5cca Compare January 9, 2020 10:46

RalfJung reviewed Jan 9, 2020

View reviewed changes

src/liballoc/collections/btree/node.rs Show resolved Hide resolved

RalfJung reviewed Jan 9, 2020

View reviewed changes

src/liballoc/collections/btree/search.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

9b92bf8

Co-Authored-By: Ralf Jung <post@ralfj.de>

rust-highfive assigned RalfJung and unassigned dtolnay Jan 9, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2020

JohnTitor mentioned this pull request Jan 9, 2020

Rollup of 10 pull requests #68067

Merged

bors merged commit 9b92bf8 into rust-lang:master Jan 9, 2020

ssomers deleted the into_key_slice_mut branch January 22, 2020 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify into_key_slice_mut #67725

Simplify into_key_slice_mut #67725

ssomers commented Dec 30, 2019 •

edited

Loading

Dylan-DPC-zz Dec 30, 2019

ssomers Dec 30, 2019 •

edited

Loading

Dylan-DPC-zz Dec 30, 2019

ssomers Dec 31, 2019

ssomers Jan 4, 2020 •

edited

Loading

RalfJung commented Dec 30, 2019

ssomers commented Dec 31, 2019

Dylan-DPC-zz commented Dec 31, 2019

ssomers commented Dec 31, 2019 •

edited

Loading

RalfJung commented Dec 31, 2019

ssomers commented Dec 31, 2019

ssomers commented Jan 1, 2020

ssomers commented Jan 4, 2020

RalfJung commented Jan 6, 2020

ssomers commented Jan 6, 2020

RalfJung commented Jan 6, 2020 •

edited

Loading

ssomers commented Jan 6, 2020 •

edited

Loading

RalfJung commented Jan 6, 2020

ssomers commented Jan 6, 2020

RalfJung commented Jan 6, 2020

ssomers commented Jan 6, 2020

RalfJung commented Jan 9, 2020

bors commented Jan 9, 2020

ssomers commented Jan 9, 2020

RalfJung commented Jan 9, 2020

ssomers commented Jan 9, 2020

RalfJung commented Jan 9, 2020

Simplify into_key_slice_mut #67725

Simplify into_key_slice_mut #67725

Conversation

ssomers commented Dec 30, 2019 • edited Loading

Dylan-DPC-zz Dec 30, 2019

Choose a reason for hiding this comment

ssomers Dec 30, 2019 • edited Loading

Choose a reason for hiding this comment

Dylan-DPC-zz Dec 30, 2019

Choose a reason for hiding this comment

ssomers Dec 31, 2019

Choose a reason for hiding this comment

ssomers Jan 4, 2020 • edited Loading

Choose a reason for hiding this comment

RalfJung commented Dec 30, 2019

ssomers commented Dec 31, 2019

Dylan-DPC-zz commented Dec 31, 2019

ssomers commented Dec 31, 2019 • edited Loading

RalfJung commented Dec 31, 2019

ssomers commented Dec 31, 2019

ssomers commented Jan 1, 2020

ssomers commented Jan 4, 2020

RalfJung commented Jan 6, 2020

ssomers commented Jan 6, 2020

RalfJung commented Jan 6, 2020 • edited Loading

ssomers commented Jan 6, 2020 • edited Loading

RalfJung commented Jan 6, 2020

ssomers commented Jan 6, 2020

RalfJung commented Jan 6, 2020

ssomers commented Jan 6, 2020

RalfJung commented Jan 9, 2020

bors commented Jan 9, 2020

ssomers commented Jan 9, 2020

RalfJung commented Jan 9, 2020

ssomers commented Jan 9, 2020

RalfJung commented Jan 9, 2020

ssomers commented Dec 30, 2019 •

edited

Loading

ssomers Dec 30, 2019 •

edited

Loading

ssomers Jan 4, 2020 •

edited

Loading

ssomers commented Dec 31, 2019 •

edited

Loading

RalfJung commented Jan 6, 2020 •

edited

Loading

ssomers commented Jan 6, 2020 •

edited

Loading