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.

Sign up for GitHub

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

Jump to bottom

Rustdoc: remove ListAttributesIter and use impl Iterator instead #92353

Merged
merged 1 commit into from
Jan 25, 2022
Merged

Rustdoc: remove ListAttributesIter and use impl Iterator instead #92353

merged 1 commit into from
Jan 25, 2022

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Dec 28, 2021 •
edited
Loading

This is a continuation of #92227.

I found that ListAttributesIter did not optimize well and replacing it with a simple impl Iterator resulted in 1-3 % instruction count wins locally.

Because I needed to use impl Iterator on a slice of AST attributes, I had to implement it using GAT + impl trait. I also have a version without GAT here, if GATs are not welcome in rustdoc :D Locally it resulted in equal performance numbers.

Can I ask for a perf. run? Thanks.

r? rust-lang/rustdoc

@rust-highfive
Copy link
Collaborator

Some changes occurred in clean/types.rs.

cc @camelid

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Dec 28, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 28, 2021
@GuillaumeGomez
Copy link
Member

The code changes look good (less code is generally a very good thing :) ). Let's see the impact on performance now.

@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 Dec 28, 2021
@bors
Copy link
Contributor

bors commented Dec 28, 2021

⌛ Trying commit 542e4dc1050ef209259bf8c878055ccd063388b8 with merge b0b6ce13300611f32c7f3dca8d5e631cc9288c85...

@bors
Copy link
Contributor

bors commented Dec 28, 2021

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

@rust-timer
Copy link
Collaborator

Queued b0b6ce13300611f32c7f3dca8d5e631cc9288c85 with parent e91ad5f, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b0b6ce13300611f32c7f3dca8d5e631cc9288c85): comparison url.

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

  • Large improvement in instruction counts (up to -3.5% on full builds of helloworld)
  • Moderate regression in instruction counts (up to 1.2% on incr-unchanged builds of deeply-nested-async)

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 Dec 28, 2021
@Kobzol Kobzol mentioned this pull request Dec 28, 2021
Merged
@GuillaumeGomez
Copy link
Member

There are very nice performance improvements! However there are max RSS regression. I think it'd be fine to ignore them when taking into account the code and performance improvements but let's see what others think.

cc @camelid

@jyn514
Copy link
Member

jyn514 commented Dec 28, 2021

I like the version without GATs better, I think using free functions instead of traits makes it easier to understand.

@Kobzol
Copy link
Contributor Author

Kobzol commented Dec 28, 2021

I liked how easy it was to use GAT and that I didn't have to change the outside code and it has gained a perf. improvement. But it's up to you :) Let me know which version should I use and I'll push it to this branch.

@Kobzol Kobzol mentioned this pull request Jan 5, 2022
Merged
@Kobzol Kobzol force-pushed the doc-attr-lists-gat branch from 542e4dc to 3a3e4b7 Compare January 15, 2022 19:51
@Kobzol
Copy link
Contributor Author

Kobzol commented Jan 15, 2022

Rebased on top of master.

@GuillaumeGomez
Copy link
Member

Thanks!

@bors: r+ rollup=iffy

@camelid
Copy link
Member

camelid commented Jan 17, 2022

@bors rollup=never (this has large perf effects)

@Kobzol
Copy link
Contributor Author

Kobzol commented Jan 24, 2022

Did this get stuck or is bors currently so busy that rollup=never PRs take a week to merge? 👀

@mati865
Copy link
Contributor

mati865 commented Jan 24, 2022

Bors has missed the comment.

@GuillaumeGomez
Copy link
Member

Let's try again then!

@bors: r+ rollup=never

@bors
Copy link
Contributor

bors commented Jan 24, 2022

📌 Commit 3a3e4b7 has been approved by GuillaumeGomez

@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 Jan 24, 2022
@bors
Copy link
Contributor

bors commented Jan 25, 2022

⌛ Testing commit 3a3e4b7 with merge 90143a689fa40607ba92bb1065399ea9dc78843f...

@bors
Copy link
Contributor

bors commented Jan 25, 2022

💔 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 Jan 25, 2022
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@Kobzol
Copy link
Contributor Author

Kobzol commented Jan 25, 2022

Looks like a spurious failure/timeout?

@GuillaumeGomez
Copy link
Member

Yup, let's retry.

@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 Jan 25, 2022
@bors
Copy link
Contributor

bors commented Jan 25, 2022

⌛ Testing commit 3a3e4b7 with merge 92ed874...

@bors
Copy link
Contributor

bors commented Jan 25, 2022

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 92ed874 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 25, 2022
@bors bors merged commit 92ed874 into rust-lang:master Jan 25, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 25, 2022
@bors bors mentioned this pull request Jan 25, 2022
Closed
7 tasks
@Kobzol Kobzol deleted the doc-attr-lists-gat branch January 25, 2022 19:33
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (92ed874): comparison url.

Summary: This benchmark run shows 45 relevant improvements 🎉 to instruction counts.

  • Average relevant improvement: -2.6%
  • Largest improvement in instruction counts: -4.2% on full builds of deeply-nested doc

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

@rustbot label: -perf-regression

@rustbot rustbot removed the perf-regression Performance regression. label Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@GuillaumeGomez GuillaumeGomez

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-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.
10 participants
@Kobzol @rust-highfive @GuillaumeGomez @rust-timer @bors @jyn514 @camelid @mati865 @rust-log-analyzer @rustbot