Skip to content

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
Collaborator

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
Collaborator

bors commented Sep 16, 2020

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

@bors
Copy link
Collaborator

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
Collaborator

bors commented Sep 17, 2020

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

@bors
Copy link
Collaborator

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
Collaborator

bors commented Sep 18, 2020

⌛ Testing commit 378b643 with merge a0925fb...

@bors
Copy link
Collaborator

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.

None yet

7 participants