BTreeMap::entry: Avoid allocating if no insertion

[frank-king]

This PR allows the VacantEntry to borrow from an empty tree with no root, and to lazily allocate a new root node when the user calls .insert(value).

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @m-ou-se (or someone else) soon.

Please see the contribution instructions for more information.

[camelid]

r? rust-lang/libs

[camelid]

r? @Amanieu
cc @ssomers

[ssomers]

Good idea, but right now, I think an insert would reallocate the root node of an emptied map (a map with an empty root node, a map whose elements have been individually removed). I would replace if map.is_empty() { and the old ensure_is_owned with a test on self.root like iter_mut, range_mut and drain_filter_inner do.

[ssomers]

I would let push return a &mut V, which is already returned by the last write in its body. And the first *mut V seems pretty superfluous.

[frank-king]

The *mut V comes from Handle::insert_fit, then forwarded by Handle::insert, and then reborrowed by Entry::insert. For this special case (inserting into a new map), I agree with you that returning &mut V directly is better.

[ssomers]

For clarity, with "the first *mut V" I meant the type annotation, in addition to the "as" cast. I was too lazy to look up the right terms (it's not type ascription or declaration); and I'm still too lazy to figure out "as" - it's a "primitive cast", says the book, but "cast" appears only twice elsewhere.

[ssomers]

I do have a physical book (O'Reilly's Programming Rust): it says the "as operator" converts, doesn't even mention casting.

[frank-king]

I messed up the *mut V you mentioned with other places and I thought you meant the conversion from &mut V to *mut V and then back to &mut V. So there has been another version discussed at #92962 (comment).

For clarity, with "the first *mut V" I meant the type annotation, in addition to the "as" cast.

Then I realised you meant the two *mut V right here in this same statement.

And the first *mut V seems pretty superfluous.

I do agree with you. Thanks for pointing out that.

[ssomers]

I see now that it makes little sense to let insert_fit (on leaf nodes) return &mut V , because it doesn't live long enough. It cannot return &'a mut V , because the return value of assume_init_mut doesn't live long enough and the borrow checker won't allow that. It cannot consume self either.

[frank-king]

I don't think so. assume_init_mut borrows from self.node.val_area_mut(self.idx), which borrows from self.node. Since self is passed by reference, it is no problem to return a &mut V which lives as long as &mut self (both of which lifetimes are eliminated).

[ssomers]

Sorry, I tried to clarify the first version of the comment and pasted into the wrong sentence. Yes, there's no problem letting it return &mut V, and I think that's slightly better, but even then, we need a &'a mut V.

[ssomers]

So I tried at https://github.com/ssomers/rust/tree/btree_prune_insert_2 to pass the &'a mut V directly back to the caller, and have one less case where the conversion to *mut V happens, but it's not worth the trouble. So instead I settled on the slight simplification #94699.

[frank-king]

            InsertResult::Fit(unsafe { &mut *(val_ref as *mut _) })

https://github.com/ssomers/rust/blob/160dd614ce752c9e7f447190273e2849a5b41867/library/alloc/src/collections/btree/node.rs#L867

Oh, I think it's even more tricky than the current version. I really thought it was totally safe until I see this unsafe block. This just splits up a single unsafe block (at the end of Entry::insert) to two, and one of which is hidden inside (in the node module).

Anyway, #94699 looks much better.

[frank-king]

but right now, I think an insert would reallocate the root node of an emptied map (a map with an empty root node, a map whose elements have been individually removed).

I'm afraid I cannot fully understand what you said. As I observed, even before this PR, inserting into an emptied allocated map via VaccantEntry::insert seems not to reallocate the root node. Map::ensure_is_owned uses Option::get_or_insert_with(Root::new), which only allocate a new node if absent.

I would replace if map.is_empty() { and the old ensure_is_owned with a test on self.root like iter_mut, range_mut and drain_filter_inner do.

I replaced map.is_empty() with map.root.is_none() in some of the tests. But I'm not sure if it is desirable. What did you mean by mentioning iter_mut, range_mut and drain_filter_inner? IterMut uses LazyLeafRange, RangeMut uses LeafRange, and DrainFilterInner uses Option<DormantMutRef>. I think they are all lazy.

[ssomers]

I'm afraid I cannot fully understand what you said. As I observed, even before this PR, inserting into an emptied allocated map via VaccantEntry::insert seems not to reallocate the root node.

With "right now" I meant that in the first form of your PR, I think it would reallocate the root of an already allocated empty map. And implicitly that it didn't do that before your PR.

I replaced map.is_empty() with map.root.is_none() in some of the tests. But I'm not sure if it is desirable.

It has pros and cons. It used to be impossible, when the unit tests where filed as integration tests, because there's no way for outside clients to see if an empty map is allocated or not. So the question hasn't come up. But that wasn't what my comment was about.

What did you mean by mentioning iter_mut, range_mut and drain_filter_inner?

Like you did in the second version of your PR: don't use ensure_is_owned, but check if the root is there yourself. Or in the case of Recover::replace, I suppose there was no point in changing it - I never really understood what that is used for.

Only now I realize the name ensure_is_owned has become nonsensical. If a map didn't "own" its root, it "shared" it with all other empty maps of the same type. It should be something like ensure_has_root.

[frank-king]

I find ensure_is_owned used in 3 places (test codes are not counted):

• in BTreeMap::append, but it checks and makes sure that both self and other is not empty first, so I think ensure_is_owned is not bad here;
  

  rust/library/alloc/src/collections/btree/map.rs

  Line 1035 in 86067bb

  let root = BTreeMap::ensure_is_owned(&mut self.root);

• in Clone::clone for BTreeMap, but for a tree with Internal node, its first edge must not be empty, and there must be a root node of out_tree, so I think it's ok here;
  

  rust/library/alloc/src/collections/btree/map.rs

  Line 215 in 86067bb

  let out_root = BTreeMap::ensure_is_owned(&mut out_tree.root);

• in Recover::replace, since the insertion must take place, it's also ok here;
  

  rust/library/alloc/src/collections/btree/map.rs

  Line 281 in 86067bb

  let root_node = Self::ensure_is_owned(&mut map.root).borrow_mut();

There is a common reason to use ensure_is_owned in the above cases: they are all "must-mutate" for the root node, i.e. whether or not the root node is allocated before, it must be allocated after this operation.

BTreeMap::entry used to use ensure_is_owned before this PR, where it is "maybe-mutate", i.e. it is possible that the root node remains empty after this operation, and it need not be allocated if it remains empty after that. In such case, I don't think using ensure_is_owned is a good idea, neither.

You mentioned the naming of ensure_is_owned isn't clear enough. I do agree that ensure_has_root has a better meaning. I think I can rename it.

[ssomers]

I made ensure_is_owned long ago to capture some repeated, relatively complicafed code. That's probably how it then got into clone, just to piggyback on some code accessing the root, because I'm sure you're right that in clone the root must be there already. When I see what's left of the body now, I would actually get rid of ensure_is_owned and inline it.

[ssomers]

Actually, I don't think ensure_has_root was a good suggestion, since the function doesn't operate on anything that "has" a root, but operates on the root itself. ensure_is_allocated would have been better. Or ensure_is_some.

On second thought, the function does/did operate on an Option, so that thing does contain a root. get_or_insert_new would be a name remarkably resembling its body.

[bors]

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

[Amanieu]

I'm not familiar enough with the btree code to review this. @ssomers would you like to review this since you worked on a lot of the btree code?

@bors delegate=ssomers

[bors]

✌️ @ssomers can now approve this pull request

[Amanieu]

@bors r+

[bors]

📌 Commit 2c3c891 has been approved by Amanieu

[bors]

⌛ Testing commit 2c3c891 with merge 078166e30b26dd26beeb1a38234f2f748109cd96...

[bors]

💔 Test failed - checks-actions

[rust-log-analyzer]

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

.......... (60/61)
          (61/61)


/checkout/src/test/rustdoc-gui/escape-key.goml escape-key... FAILED
[ERROR] (line 6) TimeoutError: waiting for selector "#search h1" failed: timeout 30000ms exceeded: for command `wait-for: "#search h1" // The search element is empty before the first search `
[ERROR] (line 7) Error: Evaluation failed: expected `content` for attribute `class` for selector `#search`, found `content hidden`: for command `assert-attribute: ("#search", {"class": "content"})`
[ERROR] (line 8) Error: Evaluation failed: expected `content hidden` for attribute `class` for selector `#main-content`, found `content`: for command `assert-attribute: ("#main-content", {"class": "content hidden"})`
[ERROR] (line 9) Error: Evaluation failed: Property `URL` (`file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/doc/test_docs/index.html`) does not end with `index.html?search=test`: for command `assert-document-property: ({"URL": "index.html?search=test"}, ENDS_WITH)`
[ERROR] (line 17) Error: Evaluation failed: expected `content` for attribute `class` for selector `#search`, found `content hidden`: for command `assert-attribute: ("#search", {"class": "content"})`
[ERROR] (line 18) Error: Evaluation failed: expected `content hidden` for attribute `class` for selector `#main-content`, found `content`: for command `assert-attribute: ("#main-content", {"class": "content hidden"})`
[ERROR] (line 19) Error: Evaluation failed: Property `URL` (`file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/doc/test_docs/index.html`) does not end with `index.html?search=test`: for command `assert-document-property: ({"URL": "index.html?search=test"}, ENDS_WITH)`
[ERROR] (line 24) Error: Evaluation failed: Property `URL` (`file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/doc/test_docs/index.html`) does not end with `index.html?search=test`: for command `assert-document-property: ({"URL": "index.html?search=test"}, [ENDS_WITH])`
[ERROR] (line 28) Error: Evaluation failed: expected `content` for attribute `class` for selector `#search`, found `content hidden`: for command `assert-attribute: ("#search", {"class": "content"})`
[ERROR] (line 29) Error: Evaluation failed: expected `content hidden` for attribute `class` for selector `#main-content`, found `content`: for command `assert-attribute: ("#main-content", {"class": "content hidden"})`
[ERROR] (line 30) Error: Evaluation failed: Property `URL` (`file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/doc/test_docs/index.html`) does not end with `index.html?search=test`: for command `assert-document-property: ({"URL": "index.html?search=test"}, [ENDS_WITH])`
[ERROR] (line 36) assert didn't fail: for command `assert-false: ".search-input:focus"`
[ERROR] (line 37) "#results a:focus" not found: for command `assert: "#results a:focus"`
Build completed unsuccessfully in 0:00:45

[Amanieu]

@bors retry

[bors]

⌛ Testing commit 2c3c891 with merge c7ce69f...

[bors]

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing c7ce69f to master...

[rust-timer]

Finished benchmarking commit (c7ce69f): comparison url.

Summary: This benchmark run did not return any relevant results. 5 results were found to be statistically significant but too small to be relevant.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression