Implement clone_from for BTreeMap and BTreeSet #66648

crgl · 2019-11-22T22:34:41Z

See #28481. This results in up to 90% speedups on simple data types when self and other are the same size, and is generally comparable or faster. Some concerns:

This implementation requires an Ord bound on the Clone implementation for BTreeMap and BTreeSet. Since these structs can only be created externally for keys with Ord implemented, this should be fine? If not, there's certainly a less safe way to do this.
Changing next_unchecked on RangeMut to return mutable key references allows for replacing the entire overlapping portion of both maps without changing the external interface in any way. However, if clone_from fails it can leave the BTreeMap in an invalid state, which might be unacceptable.

~~This probably needs an FCP since it changes a trait bound, but (as far as I know?) that change cannot break any external code.~~

rust-highfive · 2019-11-22T22:34:52Z

r? @Mark-Simulacrum

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

src/liballoc/collections/btree/map.rs

Centril · 2019-11-22T22:53:12Z

r? @scottmcm cc @bluss

Centril · 2019-11-22T22:53:56Z

@bors try @rust-timer queue

rust-timer · 2019-11-22T22:53:57Z

Awaiting bors try build completion

bors · 2019-11-22T22:54:07Z

⌛ Trying commit 8f05f02 with merge 633e948...

Implement clone_from for BTreeMap and BTreeSet See #28481. This results in up to 90% speedups on simple data types when `self` and `other` are the same size, and is generally comparable or faster. Some concerns: 1. This implementation requires an `Ord` bound on the `Clone` implementation for `BTreeMap` and `BTreeSet`. Since these structs can only be created externally for keys with `Ord` implemented, this should be fine? If not, there's certainly a less safe way to do this. 2. Changing `next_unchecked` on `RangeMut` to return mutable key references allows for replacing the entire overlapping portion of both maps without changing the external interface in any way. However, if `clone_from` fails it can leave the `BTreeMap` in an invalid state, which might be unacceptable. This probably needs an FCP since it changes a trait bound, but (as far as I know?) that change cannot break any external code.

bors · 2019-11-23T01:26:07Z

☀️ Try build successful - checks-azure
Build commit: 633e948 (633e9482caf6121c3e6152b058328f575d580124)

rust-timer · 2019-11-23T01:26:09Z

Queued 633e948 with parent 5fa0af2, future comparison URL.

src/liballoc/collections/btree/set.rs

src/liballoc/collections/btree/map.rs

JohnCSimon · 2019-11-30T05:32:51Z

Ping from triage:
@scottmcm , checks are green and it looks like the change requests have been resolved, can you please review this PR? Thanks.

JohnCSimon · 2019-12-06T04:23:55Z

Pinging again from triage:
@scottmcm - can you please review this PR?

bors · 2019-12-23T05:51:45Z

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

crgl · 2019-12-23T20:23:37Z

This has been rebased and cleaned up to account for the formatting. Is there any interest in merging it? I believe BTree collections are the last collections in the standard library without a specialized clone_from (since HashMap and HashSet are implemented externally and probably don't need it anyway). I think it would be a reasonable thing to have and I'm happy to change any part of this that needs changing, but if deriving Clone is preferred then it probably makes sense to close this PR.

Dylan-DPC-zz · 2019-12-27T12:12:01Z

r? @KodrAus

Dylan-DPC-zz · 2020-01-16T09:35:03Z

@KodrAus any updates?

Dylan-DPC-zz · 2020-01-26T19:35:29Z

r? @Amanieu

KodrAus

Thanks for your patience @crgl!

I've just left a few comments to start, but have one question about the safety of mutating keys within the map.

src/liballoc/collections/btree/map.rs

src/liballoc/tests/btree/map.rs

Amanieu · 2020-01-28T00:11:33Z

src/liballoc/collections/btree/map.rs

+                None
+            }
+        } {
+            self.split_off(&key);


I feel that a lot of duplicate work being done here. We first find the nth key, the pass it to split_off so that it can find the position of that key (again!), after which it actually splits the tree. Would it be possible to avoid doing the key lookup twice?

Not that I can see, unfortunately! However, split_off doesn't exactly find the key and then split the tree. Because split_off has a target key, it can split the tree as it's searching for the key position on the edges it descends on. In theory it would be better to have a split_after function that splits off the first n keys, but in practice the BTree doesn't have any subtree size information available (and traverses the whole tree every time it needs to recalculate length as a result). Similarly, it would definitely be more efficient if nth was specialized for BTree iterators, but I don't see an easy way to do that. Please let me know if you have any ideas about how to avoid iterating through (up to) half the tree!

My point is that you are traversing the tree twice: once in the nth call in the iterator, and once in split_off. However fixing this is likely to be very complicated, so it's fine to leave it as it is for now.

Co-Authored-By: Ashley Mannix <[email protected]>

Amanieu · 2020-01-28T20:51:52Z

r=me if @KodrAus has no other comments to make

KodrAus · 2020-01-29T10:59:05Z

@bors r=Amanieu rollup

bors · 2020-01-29T10:59:06Z

📌 Commit 6c3e477 has been approved by Amanieu

Implement clone_from for BTreeMap and BTreeSet See rust-lang#28481. This results in up to 90% speedups on simple data types when `self` and `other` are the same size, and is generally comparable or faster. Some concerns: 1. This implementation requires an `Ord` bound on the `Clone` implementation for `BTreeMap` and `BTreeSet`. Since these structs can only be created externally for keys with `Ord` implemented, this should be fine? If not, there's certainly a less safe way to do this. 2. Changing `next_unchecked` on `RangeMut` to return mutable key references allows for replacing the entire overlapping portion of both maps without changing the external interface in any way. However, if `clone_from` fails it can leave the `BTreeMap` in an invalid state, which might be unacceptable. ~This probably needs an FCP since it changes a trait bound, but (as far as I know?) that change cannot break any external code.~

@ghost

Rollup of 6 pull requests Successful merges: - #66648 (Implement clone_from for BTreeMap and BTreeSet) - #68468 (BTreeMap: tag and explain unsafe internal functions or assert preconditions) - #68626 (Use termize instead of term_size) - #68640 (Document remaining undocumented `From` implementations for IPs) - #68651 (Document `From` implementation for NonZero nums) - #68655 (Fix revision annotations in borrowck-feature-nll-overrides-migrate) Failed merges: r? @ghost

bors · 2020-01-30T05:37:56Z

⌛ Testing commit 6c3e477 with merge 3024c4e...

rust-highfive assigned Mark-Simulacrum Nov 22, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 22, 2019

Centril reviewed Nov 22, 2019

View reviewed changes

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

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

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

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

rust-highfive assigned scottmcm and unassigned Mark-Simulacrum Nov 22, 2019

Centril reviewed Nov 23, 2019

View reviewed changes

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

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

crgl force-pushed the btree-clone-from branch from b104b9a to a576335 Compare December 23, 2019 15:58

crgl added 3 commits December 23, 2019 11:03

Make RangeMut::next_unchecked() output a mutable key reference

293cdf7

Implement clone_from for BTree collections

f547978

Add test for BTreeMap::clone_from

8651aa0

crgl force-pushed the btree-clone-from branch from a576335 to 8651aa0 Compare December 23, 2019 16:03

rust-highfive assigned KodrAus and unassigned scottmcm Dec 27, 2019

rust-highfive assigned Amanieu and unassigned KodrAus Jan 26, 2020

KodrAus reviewed Jan 27, 2020

View reviewed changes

Amanieu reviewed Jan 28, 2020

View reviewed changes

crgl and others added 4 commits January 28, 2020 11:27

Format safety comment as per tidy

3caa17b

Co-Authored-By: Ashley Mannix <[email protected]>

Include empty BTreeMap in clone_from tests

60a7c94

Add private is_empty method to RangeMut

81b6f8c

Reformat truncation section of clone_from

6c3e477

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 Jan 29, 2020

Dylan-DPC-zz mentioned this pull request Jan 30, 2020

Rollup of 6 pull requests #68659

Merged

bors merged commit 6c3e477 into rust-lang:master Jan 30, 2020

crgl deleted the btree-clone-from branch January 30, 2020 14:24

ssomers mentioned this pull request Jan 30, 2020

Bundle and document 6 BTreeMap navigation algorithms #67073

Merged

ssomers mentioned this pull request Feb 16, 2020

BTree: lighten the load on Miri #68781

Merged

ssomers added a commit to ssomers/rust that referenced this pull request Feb 16, 2020

Fix comments outdated during rust-lang#66648

914b855

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement clone_from for BTreeMap and BTreeSet #66648

Implement clone_from for BTreeMap and BTreeSet #66648

crgl commented Nov 22, 2019 •

edited by Centril

Loading

rust-highfive commented Nov 22, 2019

Centril commented Nov 22, 2019

Centril commented Nov 22, 2019

rust-timer commented Nov 22, 2019

bors commented Nov 22, 2019

bors commented Nov 23, 2019

rust-timer commented Nov 23, 2019

JohnCSimon commented Nov 30, 2019

JohnCSimon commented Dec 6, 2019

bors commented Dec 23, 2019

crgl commented Dec 23, 2019

Dylan-DPC-zz commented Dec 27, 2019

Dylan-DPC-zz commented Jan 16, 2020

Dylan-DPC-zz commented Jan 26, 2020

KodrAus left a comment

Amanieu Jan 28, 2020

crgl Jan 28, 2020

Amanieu Jan 28, 2020

Amanieu commented Jan 28, 2020

KodrAus commented Jan 29, 2020

bors commented Jan 29, 2020

bors commented Jan 30, 2020

Implement clone_from for BTreeMap and BTreeSet #66648

Implement clone_from for BTreeMap and BTreeSet #66648

Conversation

crgl commented Nov 22, 2019 • edited by Centril Loading

rust-highfive commented Nov 22, 2019

Centril commented Nov 22, 2019

Centril commented Nov 22, 2019

rust-timer commented Nov 22, 2019

bors commented Nov 22, 2019

bors commented Nov 23, 2019

rust-timer commented Nov 23, 2019

JohnCSimon commented Nov 30, 2019

JohnCSimon commented Dec 6, 2019

bors commented Dec 23, 2019

crgl commented Dec 23, 2019

Dylan-DPC-zz commented Dec 27, 2019

Dylan-DPC-zz commented Jan 16, 2020

Dylan-DPC-zz commented Jan 26, 2020

KodrAus left a comment

Choose a reason for hiding this comment

Amanieu Jan 28, 2020

Choose a reason for hiding this comment

crgl Jan 28, 2020

Choose a reason for hiding this comment

Amanieu Jan 28, 2020

Choose a reason for hiding this comment

Amanieu commented Jan 28, 2020

KodrAus commented Jan 29, 2020

bors commented Jan 29, 2020

bors commented Jan 30, 2020

crgl commented Nov 22, 2019 •

edited by Centril

Loading