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

extract code path shared between FromIterator and Extend #84255

Closed
wants to merge 3 commits into from

Conversation

the8472
Copy link
Member

@the8472 the8472 commented Apr 16, 2021

This should eliminate the call to reserve on the FromIterator code path since that's redundant after the with_capacity call. So it should result in reduced IR.

I hope this will result in better compile times by itself, but if not it may still improve the numbers combined with the revert in #83797 which would increase the code weight of reserve()

@rustbot label T-libs-impl

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 16, 2021
@bjorn3
Copy link
Member

bjorn3 commented Apr 16, 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 16, 2021
@bors
Copy link
Contributor

bors commented Apr 16, 2021

⌛ Trying commit 3106144c936d272948ff0b8810fed5631313b3c5 with merge ac90c16857e21ef6d6b1345ef9256dce98e9fe32...

@bors
Copy link
Contributor

bors commented Apr 16, 2021

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

@rust-timer
Copy link
Collaborator

Queued ac90c16857e21ef6d6b1345ef9256dce98e9fe32 with parent 2faef12, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (ac90c16857e21ef6d6b1345ef9256dce98e9fe32): 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 removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 16, 2021
@the8472 the8472 force-pushed the outline-insert-loops branch from e4a49fc to 4f49480 Compare April 16, 2021 23:09
@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 28, 2021
@bors
Copy link
Contributor

bors commented May 6, 2021

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

@the8472 the8472 force-pushed the outline-insert-loops branch from 4f49480 to 4e185ae Compare May 6, 2021 18:07
@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 May 23, 2021
@bors
Copy link
Contributor

bors commented May 26, 2021

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

@crlf0710 crlf0710 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 Jun 11, 2021
@bors
Copy link
Contributor

bors commented Jun 23, 2021

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

@the8472 the8472 force-pushed the outline-insert-loops branch 2 times, most recently from f96b375 to 32e5469 Compare June 23, 2021 22:35
@crlf0710 crlf0710 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 Jul 9, 2021
@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 Jul 25, 2021
@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 Sep 27, 2021
@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 Oct 19, 2021
@JohnTitor
Copy link
Member

Has something been changed after reverting #83770? I'm seeing #83770 (comment) and would like to know if we should re-run a perf-check.

@kennytm
Copy link
Member

kennytm commented Oct 19, 2021

lgtm, but let's perf again if we have concern.

@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 Oct 19, 2021
@bors
Copy link
Contributor

bors commented Oct 19, 2021

⌛ Trying commit 32e5469d7eaea4af7d07d2ae2fad85571eb99611 with merge 5f36abf9c680c844c0922aba74bf53de36c196df...

@bors
Copy link
Contributor

bors commented Oct 19, 2021

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

@rust-timer
Copy link
Collaborator

Queued 5f36abf9c680c844c0922aba74bf53de36c196df with parent cd8b56f, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5f36abf9c680c844c0922aba74bf53de36c196df): comparison url.

Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.

  • Small improvement in instruction counts (up to -0.6% on incr-unchanged builds of ripgrep)
  • Moderate regression in instruction counts (up to 2.2% on full builds of regex)

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

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Oct 19, 2021
the8472 and others added 3 commits October 19, 2021 19:32
Previously FromIterator specialization called with_capacity() and
then delegated to Extend which called reserve().
The reserve() is only needed when extending. This should reduce the
amount of generated IR.
@the8472 the8472 force-pushed the outline-insert-loops branch from 32e5469 to 7f904fb Compare October 19, 2021 17:39
@the8472
Copy link
Member Author

the8472 commented Oct 19, 2021

let's see if this is due to changes in inlining.

@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 Oct 19, 2021
@bors
Copy link
Contributor

bors commented Oct 19, 2021

⌛ Trying commit 7f904fb with merge ba1a6f780e8a742e9ef7c9ad17635f9dc05e173b...

@bors
Copy link
Contributor

bors commented Oct 19, 2021

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

@rust-timer
Copy link
Collaborator

Queued ba1a6f780e8a742e9ef7c9ad17635f9dc05e173b with parent 1af55d1, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ba1a6f780e8a742e9ef7c9ad17635f9dc05e173b): comparison url.

Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.

  • Small improvement in instruction counts (up to -0.5% on incr-unchanged builds of ripgrep)
  • Moderate regression in instruction counts (up to 2.0% on full builds of regex)

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

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 19, 2021
@the8472
Copy link
Member Author

the8472 commented Oct 19, 2021

Looks like it's no longer worth it.

@the8472 the8472 closed this Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

10 participants