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

Add TrustedRandomAccess specialization for Vec::extend() #83770

Merged
merged 1 commit into from May 26, 2021

Conversation

the8472
Copy link
Member

@the8472 the8472 commented Apr 2, 2021

This should do roughly the same as the TrustedLen specialization but result in less IR by using __iterator_get_unchecked
instead of Iterator::for_each

Conflicting specializations are manually prioritized by grouping them under yet another helper trait.

@the8472
Copy link
Member Author

the8472 commented Apr 2, 2021

@rustbot label +T-libs-impl

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 2, 2021
@tmiasko
Copy link
Contributor

tmiasko commented Apr 2, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 2, 2021
@bors
Copy link
Contributor

bors commented Apr 2, 2021

⌛ Trying commit d3b03191639102db5aa13f0f525f1f68d11550ac with merge e808b7b361f6311b62118983e044a5f05634690d...

@bors
Copy link
Contributor

bors commented Apr 2, 2021

☀️ Try build successful - checks-actions
Build commit: e808b7b361f6311b62118983e044a5f05634690d (e808b7b361f6311b62118983e044a5f05634690d)

@rust-timer
Copy link
Collaborator

Queued e808b7b361f6311b62118983e044a5f05634690d with parent d1065e6, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (e808b7b361f6311b62118983e044a5f05634690d): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 2, 2021
@the8472
Copy link
Member Author

the8472 commented Apr 2, 2021

@Dylan-DPC this is missing an assignee

@Dylan-DPC-zz
Copy link

r? @dtolnay

This should do roughly the same as the TrustedLen specialization
but result in less IR by using __iterator_get_unchecked
instead of iterator.for_each.
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 26, 2021
@bstrie bstrie added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 12, 2021
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! The implementation looks good to me.

Would you be able to provide an example program where this produces better codegen after optimizations? Is there a simple thing I could measure with cargo bench or cargo run --release to see the improvement? (Or is that not the point of the PR?)

@dtolnay dtolnay 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-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2021
@the8472
Copy link
Member Author

the8472 commented May 23, 2021

The goal here was compile time improvements, not runtime. Although runtime might benefit in some cases too, but I haven't looked at the assembly output for this PR. When available __iterator_get_unchecked just results in less pre-optimization IR and shallower call trees, which makes things easier on llvm. The perf run seems to agree.

I can run the alloc vec benchmarks and compare assembly output to see if there's some improvement on that front if you want.

@dtolnay
Copy link
Member

dtolnay commented May 23, 2021

Ah okay, makes sense! I am not familiar enough with reading the rustc performance comparisons to make a call whether this is worthwhile. WDYT @Mark-Simulacrum?

r? @Mark-Simulacrum

@dtolnay dtolnay added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 23, 2021
@Mark-Simulacrum
Copy link
Member

Performance looks promising but I have a couple questions on the implementation (not necessarily blockers, just want to hear your thoughts).

@Mark-Simulacrum Mark-Simulacrum 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-review Status: Awaiting review from the assignee but also interested parties. labels May 26, 2021
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented May 26, 2021

📌 Commit 0202875 has been approved by Mark-Simulacrum

@bors
Copy link
Contributor

bors commented May 26, 2021

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 26, 2021
@bors
Copy link
Contributor

bors commented May 26, 2021

⌛ Testing commit 0202875 with merge 9111b8a...

@bors
Copy link
Contributor

bors commented May 26, 2021

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 9111b8a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 26, 2021
@bors bors merged commit 9111b8a into rust-lang:master May 26, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 26, 2021
@klensy
Copy link
Contributor

klensy commented May 27, 2021

@the8472
Copy link
Member Author

the8472 commented May 27, 2021

Those results are quite surprising, especially since the sign of the deltas changed in some cases.

But I guess we can revert this and I redo those changes together with #84255 which needs rebasing anyway and touches some of the same code paths, then they can be measured together.

the8472 added a commit to the8472/rust that referenced this pull request May 27, 2021
…mulacrum"

Due to a performance regression that didn't show up in the original perf run
this reverts commit 9111b8a, reversing
changes made to 9a700d2.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 30, 2021
Revert "Auto merge of rust-lang#83770 - the8472:tra-extend, r=Mark-Simulacrum"

Due to a performance regression that didn't show up in the original perf run
this reverts commit 9111b8a (rust-lang#83770), reversing
changes made to 9a700d2.

Since since is expected to have the inverse impact it should probably be rollup=never.

r? `@Mark-Simulacrum`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet