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

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

Merged
merged 3 commits into from Jan 21, 2020

Conversation

@ssomers
Copy link
Contributor

ssomers commented Dec 28, 2019

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

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 28, 2019

r? @kennytm

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

@ssomers

This comment has been minimized.

Copy link
Contributor Author

ssomers commented Dec 28, 2019

@rust-highfive rust-highfive assigned RalfJung and unassigned kennytm Dec 28, 2019
@ssomers

This comment has been minimized.

Copy link
Contributor Author

ssomers commented Dec 28, 2019

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

This comment has been minimized.

Copy link
Member

RalfJung commented Dec 29, 2019

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.

@@ -508,47 +504,9 @@ impl<'a, K, V, Type> NodeRef<marker::Mut<'a>, K, V, Type> {

impl<'a, K: 'a, V: 'a, Type> NodeRef<marker::Immut<'a>, K, V, Type> {
fn into_key_slice(self) -> &'a [K] {

This comment has been minimized.

Copy link
@RalfJung

RalfJung Dec 29, 2019

Member

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

This comment has been minimized.

Copy link
@ssomers

ssomers Dec 29, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@RalfJung

RalfJung Dec 29, 2019

Member

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

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Jan 20, 2020

Member

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

This comment has been minimized.

Copy link
Member

RalfJung commented Dec 29, 2019

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

This comment has been minimized.

Copy link
Member

RalfJung commented Dec 29, 2019

Also Cc @Gankra

@ssomers

This comment was marked as outdated.

Copy link
Contributor Author

ssomers commented Dec 29, 2019

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?

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.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Dec 29, 2019

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

This comment was marked as outdated.

Copy link
Contributor Author

ssomers commented Dec 29, 2019

Oh I see. Is that the range_search diff?

Yes, and the keys().len() calls as well.

@ssomers

This comment was marked as outdated.

Copy link
Contributor Author

ssomers commented Dec 29, 2019

Yes, and the keys().len() calls as well.

Those were for the immutable slice access, I guess. I'm not sure.

@ssomers ssomers force-pushed the ssomers:keys_start_slasher branch 2 times, most recently from cbeacc7 to de7939b Dec 30, 2019
@ssomers

This comment has been minimized.

Copy link
Contributor Author

ssomers commented Dec 30, 2019

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 ssomers force-pushed the ssomers:keys_start_slasher branch from de7939b to 125b4c3 Dec 30, 2019
@ssomers

This comment has been minimized.

Copy link
Contributor Author

ssomers commented Dec 30, 2019

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

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 30, 2019

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)

@ssomers ssomers force-pushed the ssomers:keys_start_slasher branch from 125b4c3 to 1184277 Dec 30, 2019
@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Jan 2, 2020

r? @dtolnay matching #67725

@ssomers ssomers force-pushed the ssomers:keys_start_slasher branch from a731b21 to 9e90840 Jan 10, 2020
@dtolnay dtolnay assigned Mark-Simulacrum and unassigned dtolnay Jan 16, 2020
src/liballoc/collections/btree/map.rs Show resolved Hide resolved
@@ -508,47 +504,9 @@ impl<'a, K, V, Type> NodeRef<marker::Mut<'a>, K, V, Type> {

impl<'a, K: 'a, V: 'a, Type> NodeRef<marker::Immut<'a>, K, V, Type> {
fn into_key_slice(self) -> &'a [K] {

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Jan 20, 2020

Member

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.

src/liballoc/collections/btree/search.rs Show resolved Hide resolved
@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Jan 21, 2020

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

This comment has been minimized.

Copy link
Contributor Author

ssomers commented Jan 21, 2020

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

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Jan 21, 2020

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

This comment has been minimized.

Copy link
Member

RalfJung commented Jan 21, 2020

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

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Jan 21, 2020

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

This comment has been minimized.

Copy link
Contributor Author

ssomers commented Jan 21, 2020

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

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Jan 21, 2020

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

This comment has been minimized.

Copy link
Contributor Author

ssomers commented Jan 21, 2020

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

This comment has been minimized.

Copy link
Contributor Author

ssomers commented Jan 21, 2020

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

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Jan 21, 2020

@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

This comment has been minimized.

Copy link
Contributor

bors commented Jan 21, 2020

📌 Commit 3e947ef has been approved by Mark-Simulacrum

Centril added a commit to Centril/rust that referenced this pull request Jan 21, 2020
…Simulacrum

Simplify NodeHeader by avoiding slices in BTreeMaps with shared roots

Simplify a complicated piece of code that creates slices of keys in node leaves.
bors added a commit that referenced this pull request Jan 21, 2020
Rollup of 7 pull requests

Successful merges:

 - #67686 (Simplify NodeHeader by avoiding slices in BTreeMaps with shared roots)
 - #68140 (Implement `?const` opt-out for trait bounds)
 - #68313 (Options IP_MULTICAST_TTL and IP_MULTICAST_LOOP are 1 byte on BSD)
 - #68328 (Actually pass target LLVM args to LLVM)
 - #68399 (check_match: misc unifications and ICE fixes)
 - #68415 (tidy: fix most clippy warnings)
 - #68416 (lowering: cleanup some hofs)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 21, 2020

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

@bors bors merged commit 3e947ef into rust-lang:master Jan 21, 2020
4 checks passed
4 checks passed
pr Build #20200121.28 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
@ssomers ssomers deleted the ssomers:keys_start_slasher branch Jan 21, 2020
ssomers added a commit to ssomers/rust that referenced this pull request Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.