BTreeMap: remove shared root #70111

Mark-Simulacrum · 2020-03-18T16:15:50Z

This replaces the shared root with Options in the BTreeMap code, and then slightly cleans up the node manipulation code taking advantage of the removal of the shared root. I expect that further simplification is possible, but wanted to get this posted for initial review.

Note that BTreeMap::new() continues to not allocate.

Benchmarks seem within the margin of error/unaffected, as expected for an entirely predictable branch.

 name                                 alloc-bench-a ns/iter  alloc-bench-b ns/iter  diff ns/iter  diff %  speedup
 btree::map::iter_mut_20              20                     21                                1   5.00%   x 0.95
 btree::set::clone_100                1,360                  1,439                            79   5.81%   x 0.95
 btree::set::clone_100_and_into_iter  1,319                  1,434                           115   8.72%   x 0.92
 btree::set::clone_10k                143,515                150,991                       7,476   5.21%   x 0.95
 btree::set::clone_10k_and_clear      142,792                152,916                      10,124   7.09%   x 0.93
 btree::set::clone_10k_and_into_iter  146,019                154,561                       8,542   5.85%   x 0.94

Mark-Simulacrum · 2020-03-18T16:16:46Z

I was unable to run miri as it's currently unavailable in nightly, but we can hold off landing this until miri returns (or fix potential fallout once it returns). Tests do pass locally, including under valgrind.

Mark-Simulacrum · 2020-03-18T17:58:27Z

Looks like the gdb pretty printing broke as a result of this; I am completely unfamiliar with that side of things but will try to investigate and fix.

Mark-Simulacrum · 2020-03-18T19:30:05Z

Let's r? @cuviper here as they've recently also taken a look at this code, though I would personally feel comfortable with @ssomers reviewing as well.

Mark-Simulacrum · 2020-03-18T21:11:03Z

Hm, tests did pass locally so I'm guessing this is a gdb-version specific issue? It looks like perhaps the way enums are decoded differs though. Or, as another alternative I guess, we have some layout optimization applying in stage 2 which is absent in stage 1 (where I tested).

Part of me wants to just comment out the Btree debug pretty printing entirely...

ssomers

I wonder how you came up with that gdb code, but I sure don't know how to unwrap an option there. You are aware it's being changed in #60826?

src/liballoc/collections/btree/map.rs

ssomers · 2020-03-18T21:44:03Z

src/liballoc/collections/btree/map.rs

+    front: Option<Handle<NodeRef<marker::Mut<'a>, K, V, marker::Leaf>, marker::Edge>>,
+    back: Option<Handle<NodeRef<marker::Mut<'a>, K, V, marker::Leaf>, marker::Edge>>,



Just an idea that popped up: did you consider making some static handle that could be used here, to avoid the Option(s) and the code around them? Since in that case front==back so the handles would never be dereferenced.

Yes, I did consider this. I suspect we'd run into many of the same problems as we did with the shared root (or end up having to write a bunch of boilerplate code) -- I would prefer to avoid that. There is at least once niche in the NodeRef (and Handle doesn't use it up) so I think this is essentially zero-cost anyway.

src/liballoc/collections/btree/node.rs

cuviper

In node.rs, it looks like a really nice cleanup! In map.rs, the extra unwraps and such are a little annoying, but not too bad. Overall, it looks like an improvement in safety though.

src/liballoc/collections/btree/node.rs

src/liballoc/collections/btree/map.rs

Mark-Simulacrum · 2020-03-18T23:26:10Z

I was aware of #60826, but had forgotten about it :)

It looks like that doesn't actually change the BTree printing at all pretty much so this PR would need to fix it up if we want it to work. I'm not actually personally sure how useful it is for it to work :)

Maybe @ortem has some advice on how best to work around the introduction of the Option here -- clearly my haphazard "this seems to get us what we want" is not passing CI, at least. I've pushed up a commit to attempt to debug the Some trouble now.

ssomers · 2020-03-19T14:34:32Z

If it's any comfort, your latest commit tests fine on my linux box (with gdb 8.2.1 as opposed to 8.1.0 on rust-highfive).

Mark-Simulacrum · 2020-03-19T14:37:11Z

Yeah, every commit since I tried to fix it has tested fine with my local GDB (8.3.1) too. If this doesn't work I'll try to get a 8.1 gdb copy locally I guess...

Mark-Simulacrum · 2020-03-19T17:13:49Z

@cuviper -- looks like gdb pretty printing is working now, I believe this is ready for another review pass.

cuviper · 2020-03-19T18:01:25Z

LGTM!

@bors r+

bors · 2020-03-19T18:01:26Z

📌 Commit db7a697 has been approved by cuviper

src/liballoc/collections/btree/search.rs

ssomers · 2020-03-19T18:36:03Z

You could also remove test case test_into_key_slice_with_shared_root_past_bounds and there's another outdated "shared root" comment in that file.

…cuviper BTreeMap: remove shared root This replaces the shared root with `Option`s in the BTreeMap code, and then slightly cleans up the node manipulation code taking advantage of the removal of the shared root. I expect that further simplification is possible, but wanted to get this posted for initial review. Note that `BTreeMap::new()` continues to not allocate. Benchmarks seem within the margin of error/unaffected, as expected for an entirely predictable branch. ``` name alloc-bench-a ns/iter alloc-bench-b ns/iter diff ns/iter diff % speedup btree::map::iter_mut_20 20 21 1 5.00% x 0.95 btree::set::clone_100 1,360 1,439 79 5.81% x 0.95 btree::set::clone_100_and_into_iter 1,319 1,434 115 8.72% x 0.92 btree::set::clone_10k 143,515 150,991 7,476 5.21% x 0.95 btree::set::clone_10k_and_clear 142,792 152,916 10,124 7.09% x 0.93 btree::set::clone_10k_and_into_iter 146,019 154,561 8,542 5.85% x 0.94 ```

JohnTitor · 2020-03-20T05:40:49Z

Failed in #70169 (comment)
@bors r-

This simplifies the node manipulation, as we can (in later commits) always know when traversing nodes that we are not in a shared root.

We no longer have a separate header because the shared root is gone; all code can work solely with leafs now.

This makes ensure_root_is_owned return a reference to the (now guaranteed to exist) root, allowing callers to operate on it without going through another unwrap. Unfortunately this is only rarely useful as it's frequently the case that both the length and the root need to be accessed and field-level borrows in methods don't yet exist.

Mark-Simulacrum · 2020-03-20T16:04:18Z

@bors r=cuviper Fixed up the pretty printing code (at least the test-various builder is passing locally now, and gdb 8.3 with the debugger script in this PR also does the right thing).

bors · 2020-03-20T16:04:19Z

📌 Commit bce7f6f has been approved by cuviper

…cuviper BTreeMap: remove shared root This replaces the shared root with `Option`s in the BTreeMap code, and then slightly cleans up the node manipulation code taking advantage of the removal of the shared root. I expect that further simplification is possible, but wanted to get this posted for initial review. Note that `BTreeMap::new()` continues to not allocate. Benchmarks seem within the margin of error/unaffected, as expected for an entirely predictable branch. ``` name alloc-bench-a ns/iter alloc-bench-b ns/iter diff ns/iter diff % speedup btree::map::iter_mut_20 20 21 1 5.00% x 0.95 btree::set::clone_100 1,360 1,439 79 5.81% x 0.95 btree::set::clone_100_and_into_iter 1,319 1,434 115 8.72% x 0.92 btree::set::clone_10k 143,515 150,991 7,476 5.21% x 0.95 btree::set::clone_10k_and_clear 142,792 152,916 10,124 7.09% x 0.93 btree::set::clone_10k_and_into_iter 146,019 154,561 8,542 5.85% x 0.94 ```

@ghost

Rollup of 16 pull requests Successful merges: - rust-lang#65097 (Make std::sync::Arc compatible with ThreadSanitizer) - rust-lang#69033 (Use generator resume arguments in the async/await lowering) - rust-lang#69997 (add `Option::{zip,zip_with}` methods under "option_zip" gate) - rust-lang#70038 (Remove the call that makes miri fail) - rust-lang#70058 (can_begin_literal_maybe_minus: `true` on `"-"? lit` NTs.) - rust-lang#70111 (BTreeMap: remove shared root) - rust-lang#70139 (add delay_span_bug to TransmuteSizeDiff, just to be sure) - rust-lang#70165 (Remove the erase regions MIR transform) - rust-lang#70166 (Derive PartialEq, Eq and Hash for RangeInclusive) - rust-lang#70176 (Add tests for rust-lang#58319 and rust-lang#65131) - rust-lang#70177 (Fix oudated comment for NamedRegionMap) - rust-lang#70184 (expand_include: set `.directory` to dir of included file.) - rust-lang#70187 (more clippy fixes) - rust-lang#70188 (Clean up E0439 explanation) - rust-lang#70189 (Abi::is_signed: assert that we are a Scalar) - rust-lang#70194 (#[must_use] on split_off()) Failed merges: r? @ghost

@Mark-Simulacrum

…r=Mark-Simulacrum Test and fix gdb pretty printing more Over time I had oversimplified the test case for rust-lang#68098: it does not have an internal node to print so it does not test what it pretend to test. And then I also realized not spotting the same mistake reviewing rust-lang#70111, and more likely to occur in the wild. Now, both test cases fail if you put back the flawed python code. r? @Mark-Simulacrum

@Mark-Simulacrum

…Mark-Simulacrum Test and fix gdb pretty printing more Over time I had oversimplified the test case for rust-lang#68098: it does not have an internal node to print so it did not test what it pretended to test. And then I also realized not spotting the same mistake reviewing rust-lang#70111, and more likely to occur in the wild. Now, both test cases fail if you put back the flawed python code. r? @Mark-Simulacrum

This comment has been minimized.

Sign in to view

rust-highfive assigned cuviper Mar 18, 2020

This comment has been minimized.

Sign in to view

ssomers reviewed Mar 18, 2020

View reviewed changes

cuviper reviewed Mar 18, 2020

View reviewed changes

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

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

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

Mark-Simulacrum force-pushed the btree-no-shared branch from 5175860 to 0b599bf Compare March 18, 2020 23:26

This comment has been minimized.

Sign in to view

Mark-Simulacrum force-pushed the btree-no-shared branch from 0b599bf to 4624456 Compare March 19, 2020 13:34

This comment has been minimized.

Sign in to view

Mark-Simulacrum force-pushed the btree-no-shared branch from 4624456 to db7a697 Compare March 19, 2020 14:53

ssomers mentioned this pull request Mar 19, 2020

Test gdb pretty printing more and fix overzealous type substitution #68098

Merged

Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 19, 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 Mar 19, 2020

ssomers reviewed Mar 19, 2020

View reviewed changes

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

Dylan-DPC-zz mentioned this pull request Mar 19, 2020

Rollup of 9 pull requests #70169

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 20, 2020

Mark-Simulacrum added 7 commits March 20, 2020 09:42

Replace shared root with optional root

e61f126

This simplifies the node manipulation, as we can (in later commits) always know when traversing nodes that we are not in a shared root.

Remove shared root code and assertions from BTree nodes

1c44f85

Make functions dependent only on shared root avoidance safe

3c04fda

Drop NodeHeader type from BTree code

54b7c38

We no longer have a separate header because the shared root is gone; all code can work solely with leafs now.

Update test commentary for shared root removal

4d85314

Fix debugger pretty printing of BTrees

bce7f6f

Mark-Simulacrum force-pushed the btree-no-shared branch from db7a697 to bce7f6f Compare March 20, 2020 16:02

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 20, 2020

Dylan-DPC-zz mentioned this pull request Mar 21, 2020

Rollup of 11 pull requests #70202

Closed

Centril mentioned this pull request Mar 21, 2020

Rollup of 17 pull requests #70203

Closed

Centril mentioned this pull request Mar 21, 2020

Rollup of 16 pull requests #70205

Merged

bors merged commit 9d9e381 into rust-lang:master Mar 21, 2020

ssomers mentioned this pull request Mar 22, 2020

Test and fix gdb pretty printing more #70282

Merged

Mark-Simulacrum mentioned this pull request Apr 1, 2020

Permit reordering LeafNode fields #70675

Closed

ssomers mentioned this pull request Feb 9, 2021

BTree: adapt range support functions to the root's optionality #81935

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BTreeMap: remove shared root #70111

BTreeMap: remove shared root #70111

Mark-Simulacrum commented Mar 18, 2020 •

edited

Loading

Mark-Simulacrum commented Mar 18, 2020

This comment has been minimized.

Mark-Simulacrum commented Mar 18, 2020

Mark-Simulacrum commented Mar 18, 2020

This comment has been minimized.

Mark-Simulacrum commented Mar 18, 2020

ssomers left a comment

ssomers Mar 18, 2020

Mark-Simulacrum Mar 18, 2020

cuviper left a comment

Mark-Simulacrum commented Mar 18, 2020

This comment has been minimized.

ssomers commented Mar 19, 2020 •

edited

Loading

Mark-Simulacrum commented Mar 19, 2020

This comment has been minimized.

Mark-Simulacrum commented Mar 19, 2020

cuviper commented Mar 19, 2020

bors commented Mar 19, 2020

ssomers commented Mar 19, 2020

JohnTitor commented Mar 20, 2020

Mark-Simulacrum commented Mar 20, 2020

bors commented Mar 20, 2020

		front: Option<Handle<NodeRef<marker::Mut<'a>, K, V, marker::Leaf>, marker::Edge>>,
		back: Option<Handle<NodeRef<marker::Mut<'a>, K, V, marker::Leaf>, marker::Edge>>,

BTreeMap: remove shared root #70111

BTreeMap: remove shared root #70111

Conversation

Mark-Simulacrum commented Mar 18, 2020 • edited Loading

Mark-Simulacrum commented Mar 18, 2020

This comment has been minimized.

Mark-Simulacrum commented Mar 18, 2020

Mark-Simulacrum commented Mar 18, 2020

This comment has been minimized.

Mark-Simulacrum commented Mar 18, 2020

ssomers left a comment

Choose a reason for hiding this comment

ssomers Mar 18, 2020

Choose a reason for hiding this comment

Mark-Simulacrum Mar 18, 2020

Choose a reason for hiding this comment

cuviper left a comment

Choose a reason for hiding this comment

Mark-Simulacrum commented Mar 18, 2020

This comment has been minimized.

ssomers commented Mar 19, 2020 • edited Loading

Mark-Simulacrum commented Mar 19, 2020

This comment has been minimized.

Mark-Simulacrum commented Mar 19, 2020

cuviper commented Mar 19, 2020

bors commented Mar 19, 2020

ssomers commented Mar 19, 2020

JohnTitor commented Mar 20, 2020

Mark-Simulacrum commented Mar 20, 2020

bors commented Mar 20, 2020

Mark-Simulacrum commented Mar 18, 2020 •

edited

Loading

ssomers commented Mar 19, 2020 •

edited

Loading