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 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

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 20, 2019

r? @cramertj

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

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Dec 20, 2019

@rust-highfive rust-highfive assigned Gankra and unassigned cramertj Dec 20, 2019
@ssomers

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

Centril commented Dec 21, 2019

@ssomers ssomers force-pushed the ssomers:#67438 branch from 5982b7d to 2b2667b Dec 22, 2019
@ssomers

This comment has been minimized.

Copy link
Contributor Author

ssomers commented Dec 22, 2019

Added more unit testing

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 23, 2019

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

@ssomers ssomers force-pushed the ssomers:#67438 branch from 2b2667b to 553329d Dec 23, 2019
@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Dec 23, 2019

The job x86_64-gnu-llvm-7 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-23T08:27:43.7166642Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-23T08:27:43.7184217Z ##[command]git config gc.auto 0
2019-12-23T08:27:43.7189166Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-23T08:27:43.7193837Z ##[command]git config --get-all http.proxy
2019-12-23T08:27:43.7196241Z ##[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/67459/merge:refs/remotes/pull/67459/merge

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)

@ssomers ssomers force-pushed the ssomers:#67438 branch from 553329d to 21b7d77 Dec 23, 2019
@RalfJung

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

RalfJung commented Dec 23, 2019

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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 ssomers force-pushed the ssomers:#67438 branch from 21b7d77 to 1b73374 Dec 24, 2019
@ssomers

This comment has been minimized.

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 ssomers:#67438 branch 3 times, most recently from 39af96f to 5c3d8cc Dec 24, 2019
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

This comment has been minimized.

Copy link
@RalfJung

RalfJung Dec 26, 2019

Member

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

This comment has been minimized.

Copy link
@ssomers

ssomers Dec 26, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@RalfJung

RalfJung Dec 26, 2019

Member

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.

This comment has been minimized.

Copy link
@ssomers

ssomers Dec 26, 2019

Author Contributor

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

This comment has been minimized.

Copy link
Member

RalfJung commented Dec 26, 2019

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 ssomers force-pushed the ssomers:#67438 branch from c799e8b to e3c814e Dec 26, 2019
@ssomers

This comment has been minimized.

Copy link
Contributor Author

ssomers commented Dec 26, 2019

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

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Dec 26, 2019

Looking good, thanks a lot!

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 26, 2019

📌 Commit e3c814e has been approved by RalfJung

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

This comment has been minimized.

Copy link
Contributor

bors commented Dec 28, 2019

⌛️ Testing commit e3c814e with merge 54d8ccd...

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.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 28, 2019

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

This comment has been minimized.

Copy link
Contributor

bors commented Dec 28, 2019

💔 Test failed - checks-azure

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Dec 28, 2019

curl: (7) Couldn't connect to server

Seems spurious. @bors retry

@ssomers

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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 label Dec 28, 2019
@bors bors merged commit e3c814e into rust-lang:master Dec 28, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20191226.34 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
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 ssomers:#67438 branch Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.