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: Cleanup clean::Impl and other parts of clean #90675

Merged
merged 10 commits into from
Nov 8, 2021
Merged

rustdoc: Cleanup clean::Impl and other parts of clean #90675

merged 10 commits into from
Nov 8, 2021

Conversation

camelid
Copy link
Member

@camelid camelid commented Nov 7, 2021 •
edited
Loading

This PR cleans up and reduces the size of clean::Impl, makes some other small performance improvements, and removes some Clean impls that are either unnecessary or potentially confusing.

r? @jyn514

@camelid
rustdoc: Remove redundant Impl.span field
c32ee54 
It can be computed on-demand in `Item::span()`.
@camelid
rustdoc: Refactor Impl.{synthetic,blanket_impl} into enum
7b7023c 
This change has two advantages:

1. It makes the possible states clearer, and it makes it impossible to
   construct invalid states, such as a blanket impl that is also an auto
   trait impl.

2. It shrinks the size of `Impl` a bit, since now there is only one
   field, rather than two.
@camelid
Use an enum to record polarity in clean::Impl
543aea6
@camelid
rustdoc: Use ty::ImplPolarity instead of custom enum
120ef36
@camelid
Remove Clean impl for Option<T>
750568d
@camelid
Remove Clean impl for IndexVec<T>
efe4d7c
@camelid
Remove Clean impl for Vec<T>
72b0a57
@camelid
Remove unused Clean impl for Rc<T>
d079977
@camelid
Remove Clean impl for &T
7c7bf45
@camelid camelid added C-cleanup Category: PRs that clean code up or issues documenting cleanup. I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. I-compiletime Issue: Problems and improvements with respect to compile times. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Nov 7, 2021
@camelid
Copy link
Member Author

camelid commented Nov 7, 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 Nov 7, 2021
@bors
Copy link
Contributor

bors commented Nov 7, 2021

⌛ Trying commit 7c7bf45 with merge f6043fd4839769bd32477215f74a9315cbbb8e74...

@bors
Copy link
Contributor

bors commented Nov 7, 2021

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

@rust-timer
Copy link
Collaborator

Queued f6043fd4839769bd32477215f74a9315cbbb8e74 with parent fecfc0e, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f6043fd4839769bd32477215f74a9315cbbb8e74): comparison url.

Summary: This change led to small relevant improvements 🎉 in compiler performance.

  • Small improvement in instruction counts (up to -0.7% on full builds of deeply-nested)

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.

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

@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 Nov 7, 2021
jyn514
jyn514 reviewed Nov 8, 2021
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Why did you remove the Clean impls that was used? That seems like a lot of churn for not a ton of benefit ... the rest of the commits are great though :)

src/librustdoc/clean/types.rs Outdated Show resolved Hide resolved
src/librustdoc/clean/mod.rs Show resolved Hide resolved
@jyn514 jyn514 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 Nov 8, 2021
@jyn514 jyn514 self-assigned this Nov 8, 2021
@camelid
Copy link
Member Author

camelid commented Nov 8, 2021

Why did you remove the Clean impls that was used? That seems like a lot of churn for not a ton of benefit ...

Overall, the reasons are as follows:

  1. I'd rather the code be more explicit about what is happening (e.g., that a Vec is being mapped over).
  2. These removals are small steps in the direction of getting rid of Clean altogether, or at least significantly reducing its use.
  3. Unused trait impls are not detected by the compiler and can thus accumulate. Removing them proactively prevents this from happening ;)

I also have specific reasons for each removed impl (except for the unused Rc<T> impl, and the &T impl that could be removed just by skipping an & in one spot):

  • Option: rustdoc: Cleanup clean::Impl and other parts of clean #90675 (comment)
  • IndexVec: It was only used in one place, so it seems better to just write the code there.
  • Vec: Like with Option, I'd prefer the code be more explicit about the mapping and what impl is being dispatched to. Also, this removal uncovered some places that were collecting into a Vec and then running clean on that Vec, which causes unnecessary allocations and re-traversals. Forcing a manual map helps prevents situations like this from happening in the first place.

@camelid camelid 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 Nov 8, 2021
@camelid camelid marked this pull request as ready for review November 8, 2021 02:11
@jyn514
Copy link
Member

jyn514 commented Nov 8, 2021

r=me with #90675 (comment) done :)

@bors rollup=never

@camelid
rustdoc: Remove top-level wrappers for ImplKind methods
b5817fa 
The `ImplKind` methods can just be used directly instead.
@camelid
Copy link
Member Author

camelid commented Nov 8, 2021

Done!

@bors r=jyn514 rollup=never

[you can never say rollup=never too many times ;)]

@bors
Copy link
Contributor

bors commented Nov 8, 2021

📌 Commit b5817fa has been approved by jyn514

@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 Nov 8, 2021
@bors
Copy link
Contributor

bors commented Nov 8, 2021

⌛ Testing commit b5817fa with merge 3e3890c...

@bors
Copy link
Contributor

bors commented Nov 8, 2021

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing 3e3890c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 8, 2021
@bors bors merged commit 3e3890c into rust-lang:master Nov 8, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 8, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3e3890c): comparison url.

Summary: This change led to moderate relevant improvements 🎉 in compiler performance.

  • Moderate improvement in instruction counts (up to -0.7% on full builds of deeply-nested)

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

@rustbot label: -perf-regression

@camelid camelid deleted the cleanup-impl branch November 8, 2021 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

Assignees

@jyn514 jyn514

Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. I-compiletime Issue: Problems and improvements with respect to compile times. 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.58.0
Development

Successfully merging this pull request may close these issues.
5 participants
@camelid @rust-timer @bors @jyn514 @rustbot