Skip to content
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

prune ill-conceived BTreeMap iter_mut assertion and test its mutability #67459

Merged
merged 1 commit into from
Dec 28, 2019

Conversation

ssomers
Copy link
Contributor

@ssomers ssomers commented Dec 20, 2019

Proposal to deal with #67438 (and I'm more sure now that this is the right thing to do).
Passes testing with miri.

@rust-highfive
Copy link
Collaborator

r? @cramertj

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 20, 2019
@cramertj
Copy link
Member

@bors r? @Gankra

@rust-highfive rust-highfive assigned Gankra and unassigned cramertj Dec 20, 2019
@ssomers
Copy link
Contributor Author

ssomers commented Dec 20, 2019

I also wondered why so much effort went into setting up a slice for an immutable map, and there's only one genuine use: search_linear. One extra line there removes the need for the hack inspiring this assert and saves 50 lines in node.rs, passes all tests too and is probably a bit faster:

c:\Users\stein\Documents\GitHub\rust>cargo benchcmp old1.txt new1.txt --threshold 3
 name                                           old1.txt ns/iter  new1.txt ns/iter  diff ns/iter   diff %  speedup
 btree::map::find_seq_100                       18                17                          -1   -5.56%   x 1.06
 btree::map::first_and_last_100                 37                40                           3    8.11%   x 0.92
 btree::map::insert_rand_100                    35                27                          -8  -22.86%   x 1.30
 btree::map::insert_rand_10_000                 35                27                          -8  -22.86%   x 1.30
 btree::map::insert_seq_100                     45                40                          -5  -11.11%   x 1.12
 btree::map::iter_1000                          2,764             2,666                      -98   -3.55%   x 1.04
 btree::map::iter_20                            42                40                          -2   -4.76%   x 1.05
 btree::set::difference_random_100_vs_100       720               688                        -32   -4.44%   x 1.05
 btree::set::difference_random_100_vs_10k       2,486             2,903                      417   16.77%   x 0.86
 btree::set::intersection_100_neg_vs_10k_pos    29                30                           1    3.45%   x 0.97
 btree::set::intersection_100_pos_vs_100_neg    24                25                           1    4.17%   x 0.96
 btree::set::intersection_10k_pos_vs_10k_neg    32                31                          -1   -3.12%   x 1.03
 btree::set::intersection_random_100_vs_100     637               589                        -48   -7.54%   x 1.08
 btree::set::intersection_random_100_vs_10k     2,348             2,722                      374   15.93%   x 0.86
 btree::set::intersection_random_10k_vs_100     2,294             2,477                      183    7.98%   x 0.93
 btree::set::intersection_staggered_100_vs_10k  2,322             2,133                     -189   -8.14%   x 1.09
 btree::set::is_subset_100_vs_100               378               400                         22    5.82%   x 0.95
 btree::set::is_subset_10k_vs_10k               38,394            40,790                   2,396    6.24%   x 0.94

@Centril
Copy link
Contributor

Centril commented Dec 21, 2019

cc @RalfJung

@ssomers
Copy link
Contributor Author

ssomers commented Dec 22, 2019

Added more unit testing

@bors
Copy link
Contributor

bors commented Dec 23, 2019

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

@rust-highfive

This comment has been minimized.

@RalfJung
Copy link
Member

RalfJung commented Dec 23, 2019

I also wondered why so much effort went into setting up a slice for an immutable map, and there's only one genuine use: search_linear. One extra line there removes the need for the hack inspiring this assert and saves 50 lines in node.rs, passes all tests too and is probably a bit faster

Back then I tried to keep the code as close as possible to how it was, so I didn't mess with the slices. But if we can get rid of them and thus get rid of the entirely awful keys_start thing, then I am all in favor of that!

@ssomers
Copy link
Contributor Author

ssomers commented Dec 23, 2019

There are at least two argument for keeping off my "radical" slice slashing experiment:

  • Nobody is sure it's correct. Miri is happy (on one platform), but there are not many unit tests to begin with.
  • The current code is more symmetric, slices all around, and slices are always necessary for the mutable variants. The mutable variants are easier because there's no shared root involved (for instance, into_key_slice_mut doesn't have to deal with is_shared_root; if it had to, returning a &mut [] for callers to play with wouldn't be the full story).

Still, I think it's probably best to keep the slices, but precondition out shared roots. It eliminates keys_start and just takes a tiny test in search_linear to bail out on empty trees. As in this alternative.

@RalfJung
Copy link
Member

Nobody is sure it's correct.

Well, and who's sure the current code is correct? ;)

The current code is more symmetric, slices all around, and slices are always necessary for the mutable variants. The mutable variants are easier because there's no shared root involved (for instance, into_key_slice_mut doesn't have to deal with is_shared_root; if it had to, returning a &mut [] for callers to play with wouldn't be the full story).

I was wondering about the into_key_slice_mut in your proposal where it asserts that this is not the shared root... why is that correct? We can still mutably iterate over the empty BTreeMap, for example, so some of these might be reachable with a shared root.

@ssomers
Copy link
Contributor Author

ssomers commented Dec 23, 2019

why is that correct

So it's not correct, and returning a temporary is the full story because in that case we're not going to mutate the key nor the key slice.

@ssomers
Copy link
Contributor Author

ssomers commented Dec 23, 2019

No, no, no, that last point doesn't make sense either. Must reboot myself and find sanity.

@ssomers
Copy link
Contributor Author

ssomers commented Dec 23, 2019

Sorry for the confusion. Slightly saner self says:

  • The reason for the failing asserts (debug_asserts brought to life in the alternative for this PR) is a last minute change I did in search_linear "to minimize differences".
  • The test I remembered that already existed was get_mut. It doesn't and shouldn't do ensure_root_is_owned. In the existing code, it does visit into_key_slice_mut on the shared root, and thus returning a temporary is the full story because we're not going to mutate the key slice. But get_mut relies on the same tree search as get and therefore, in both alternatives, the tweak in search_linear ensures that into_key_slice_mut no longer has to deal with the shared root either.
  • So I think my into_key_slice_mut is correct after all, but I did not yet check the other paths to it thoroughly again. Iteration has nothing to do with this, iteration only comes down in node.rs if there is any KV handle to visit, and then the tree can't be empty.

@ssomers
Copy link
Contributor Author

ssomers commented Dec 24, 2019

The new commit here only brings more and smarter testing, I haven't put back any assertion.
The simplest alternative getting rid of keys_start is corrected and the radical alternative is still okay.

@ssomers ssomers force-pushed the #67438 branch 3 times, most recently from 39af96f to 5c3d8cc Compare December 24, 2019 14:47
if mem::align_of::<K>() > mem::align_of::<LeafNode<(), ()>>() && self.is_shared_root() {
// aligned, the offset of a correctly typed `keys_start` might be outside
// the bounds of the allocated header! So we do an alignment check first,
// that will be evaluated at compile-time, and only do any run-time check
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be "alignment and size check" now, I guess?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's clear enough. In my mind, it's about the "alignment" of the keys_start field, so both the alignment of the struct and the offset of the field within the struct (which we measure through struct size). I guess "alignment" is a stretch here, but "alignment and size" is a bit of an implementation detail.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check literally looks at alignment and size, so it seems odd to call it an "alignment check". And of course this is an implementation detail, we are knee-deep inside a private method's unsafe code here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that the sentence that brings the important point across about compile-time vs. run-time checks shouldn't elaborate or repeat the steps too much.

In any case, I have now substantially rearranged points.

@RalfJung
Copy link
Member

The PR now seems fine to me, modulo nits. I assume you ran tests with debug assertions enabled?

The new commit here only brings more and smarter testing, I haven't put back any assertion.
The simplest alternative getting rid of keys_start is corrected and the radical alternative is still okay.

I really like the simplest alternative, but why can that work? You have not gotten rid of a single caller of into_key_slice_mut; do we really already not call that on an empty BTreeMap? That seems odd to me.

@ssomers
Copy link
Contributor Author

ssomers commented Dec 26, 2019

Meanwhile Miri and 32/64 bit Linux & Windows all pass

@RalfJung
Copy link
Member

Looking good, thanks a lot!

@bors r+

@bors
Copy link
Contributor

bors commented Dec 26, 2019

📌 Commit e3c814e has been approved by RalfJung

@bors 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 Dec 26, 2019
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Dec 26, 2019
prune ill-conceived BTreeMap iter_mut assertion and test its mutability

Proposal to deal with rust-lang#67438 (and I'm more sure now that this is the right thing to do).
Passes testing with miri.
bors added a commit that referenced this pull request Dec 26, 2019
Rollup of 12 pull requests

Successful merges:

 - #67112 (Refactor expression parsing thoroughly)
 - #67192 (Various const eval and pattern matching ICE fixes)
 - #67287 (typeck: note other end-point when checking range pats)
 - #67459 (prune ill-conceived BTreeMap iter_mut assertion and test its mutability)
 - #67576 (reuse `capacity` variable in slice::repeat)
 - #67602 (Use issue = "none" instead of "0" in intrinsics)
 - #67614 (Set callbacks globally)
 - #67617 (Remove `compiler_builtins_lib` documentation)
 - #67629 (Remove redundant link texts)
 - #67632 (Convert collapsed to shortcut reference links)
 - #67633 (Update .mailmap)
 - #67635 (Document safety of Path casting)

Failed merges:

r? @ghost
@bors
Copy link
Contributor

bors commented Dec 28, 2019

⌛ Testing commit e3c814e with merge 54d8ccd5c7c02f9bd56cfac8200c38e1fbcefa8d...

@rust-highfive
Copy link
Collaborator

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.

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)

@bors
Copy link
Contributor

bors commented Dec 28, 2019

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 28, 2019
@RalfJung
Copy link
Member

curl: (7) Couldn't connect to server

Seems spurious. @bors retry

@bors 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 Dec 28, 2019
@ssomers
Copy link
Contributor Author

ssomers commented Dec 28, 2019

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". At least that comment is wrong, in two ways:

  • In general, we (as a NodeRef) can be mutable and still have a shared root.
  • Specifically, we really can be the shared root here, as the unit testing with get_mut on an empty map demonstrates, though only if we've made sure up front that "accessing this as a leaf is okay".

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? Or worse, that on some platform with harder checks than Miri on x86_64 Linux, get_mut on an empty map will crash?

@bors
Copy link
Contributor

bors commented Dec 28, 2019

⌛ Testing commit e3c814e with merge e39ae6f...

bors added a commit that referenced this pull request Dec 28, 2019
prune ill-conceived BTreeMap iter_mut assertion and test its mutability

Proposal to deal with #67438 (and I'm more sure now that this is the right thing to do).
Passes testing with miri.
@bors
Copy link
Contributor

bors commented Dec 28, 2019

☀️ Test successful - checks-azure
Approved by: RalfJung
Pushing e39ae6f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 28, 2019
@bors bors merged commit e3c814e into rust-lang:master Dec 28, 2019
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 9, 2020
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
@ssomers ssomers deleted the #67438 branch January 22, 2020 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants