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

BTreeMap: avoid slices even more #76790

Merged
merged 1 commit into from
Sep 18, 2020

Conversation

ssomers
Copy link
Contributor

@ssomers ssomers commented Sep 16, 2020

Epilogue to #73971: it seems the compiler is unable to realize that creating a slice and get_unchecked-ing one element is a simple fetch. So try to spell it out for the only remaining but often invoked case.

Also, the previous code doesn't seem fair game to me, using get_unchecked to reach beyond the end of a slice. Although the local function slice_insert also does that.

r? @Mark-Simulacrum

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 16, 2020
@ssomers
Copy link
Contributor Author

ssomers commented Sep 16, 2020

The btree benchmarks of mutable behaviour rely heavily on push to implement clone and seem happy:

 name                                        old ns/iter  new ns/iter  diff ns/iter   diff %  speedup
 btree::map::clone_slim_100_and_clear        2,526        2,020                -506  -20.03%   x 1.25
 btree::map::clone_slim_100_and_drain_all    3,859        3,345                -514  -13.32%   x 1.15
 btree::map::clone_slim_100_and_drain_half   3,293        2,773                -520  -15.79%   x 1.19
 btree::map::clone_slim_100_and_into_iter    2,575        1,953                -622  -24.16%   x 1.32
 btree::map::clone_slim_100_and_pop_all      3,669        3,130                -539  -14.69%   x 1.17
 btree::map::clone_slim_100_and_remove_all   4,496        3,983                -513  -11.41%   x 1.13
 btree::map::clone_slim_100_and_remove_half  3,346        2,797                -549  -16.41%   x 1.20
 btree::map::find_seq_10_000                 38           40                      2    5.26%   x 0.95
 btree::map::range_included_excluded         422,740      449,350            26,610    6.29%   x 0.94
 btree::map::range_included_included         426,430      464,617            38,187    8.96%   x 0.92
 btree::set::is_subset_100_vs_10k            1,164        1,242                  78    6.70%   x 0.94

@Mark-Simulacrum
Copy link
Member

I think I recall seeing this pattern and being worried about it, so this definitely looks good to me.

@bors r+ rollup=never (perf effects not unlikely)

@bors
Copy link
Contributor

bors commented Sep 16, 2020

📌 Commit 378b643 has been approved by Mark-Simulacrum

@bors 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 Sep 16, 2020
@jyn514 jyn514 added I-slow Issue: Problems and improvements with respect to performance of generated code. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Sep 16, 2020
@bors
Copy link
Contributor

bors commented Sep 16, 2020

⌛ Testing commit 378b643 with merge 9dfeaa9c374c596c91380f6aa857262d23169fb0...

@bors
Copy link
Contributor

bors commented Sep 17, 2020

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 17, 2020
@tmandry
Copy link
Member

tmandry commented Sep 17, 2020

@bors retry

@bors 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 Sep 17, 2020
@bors
Copy link
Contributor

bors commented Sep 17, 2020

⌛ Testing commit 378b643 with merge 999503af5648bb816ae73899f12ee59ae8563280...

@bors
Copy link
Contributor

bors commented Sep 17, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 17, 2020
@ssomers
Copy link
Contributor Author

ssomers commented Sep 17, 2020

More timeouts, if you ask me. [ERROR] ncurses: download failed.

@Mark-Simulacrum
Copy link
Member

@bors retry

@bors 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 Sep 17, 2020
@bors
Copy link
Contributor

bors commented Sep 18, 2020

⌛ Testing commit 378b643 with merge a0925fb...

@bors
Copy link
Contributor

bors commented Sep 18, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: Mark-Simulacrum
Pushing a0925fb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 18, 2020
@bors bors merged commit a0925fb into rust-lang:master Sep 18, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 18, 2020
@ssomers ssomers deleted the btree_slice_slasher_returns branch September 18, 2020 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-slow Issue: Problems and improvements with respect to performance of generated code. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants