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

ty: remove obsolete pretty printer #76027

Merged

Conversation

davidtwco
Copy link
Member

@davidtwco davidtwco commented Aug 28, 2020

Fixes #61139.

This PR removes the obsolete printer and replaces all uses of it with FmtPrinter. Of the replaced uses, all but one use was in debug! logging, two cases were notable:

  • MonoItem::to_string is used in -Z print-mono-items and therefore affects the output of all codegen-units tests (which have been updated).
  • DefPathBasedNames was used in librustc_codegen_llvm/type_of.rs with LLVMStructCreateNamed and that'll now get different values, but nothing will break as a result of this.

cc @eddyb (whom I've discussed this with)

@rust-highfive
Copy link
Collaborator

r? @eddyb

(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 Aug 28, 2020
@davidtwco davidtwco force-pushed the issue-61139-remove-obsolete-pretty-printer branch from ada69f9 to 5033987 Compare August 28, 2020 14:21
@eddyb
Copy link
Member

eddyb commented Aug 28, 2020

cc @oli-obk @nikomatsakis (for the changes in codegen-units tests - which should probably be made into UI tests tbh)

Would be neat if we could have @michaelwoerister take a quick look to tell us if we're off the mark here, but likely won't happen.

@davidtwco davidtwco force-pushed the issue-61139-remove-obsolete-pretty-printer branch from 5033987 to f0c548d Compare August 28, 2020 17:23
@davidtwco davidtwco force-pushed the issue-61139-remove-obsolete-pretty-printer branch from f0c548d to fdf401d Compare August 28, 2020 22:44
@davidtwco davidtwco force-pushed the issue-61139-remove-obsolete-pretty-printer branch from fdf401d to 032408f Compare August 29, 2020 15:08
Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

r=me on the implementation (not sure if you want to wait for someone to give their opinion on the incremental tests? they seem fine to me, at least)

@davidtwco
Copy link
Member Author

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Aug 30, 2020

📌 Commit 032408fdfe71597546141dbe89b6213d90d43062 has been approved by eddyb

@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 Aug 30, 2020
This commit removes the obsolete printer and replaces all uses of it
with `FmtPrinter`. Of the replaced uses, all but one use was in `debug!`
logging, two cases were notable:

- `MonoItem::to_string` is used in `-Z print-mono-items` and therefore
  affects the output of all codegen-units tests.
- `DefPathBasedNames` was used in `librustc_codegen_llvm/type_of.rs`
  with `LLVMStructCreateNamed` and that'll now get different values, but
  this should result in no functional change.

Signed-off-by: David Wood <david@davidtw.co>
@davidtwco davidtwco force-pushed the issue-61139-remove-obsolete-pretty-printer branch from 032408f to 6ff471b Compare August 30, 2020 17:59
@davidtwco
Copy link
Member Author

davidtwco commented Aug 30, 2020

@bors r=eddyb (rebase was only after #74862)

@bors
Copy link
Contributor

bors commented Aug 30, 2020

📌 Commit 6ff471b has been approved by eddyb

@bors

This comment has been minimized.

@bors

This comment has been minimized.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Aug 30, 2020
…te-pretty-printer, r=eddyb

ty: remove obsolete pretty printer

Fixes rust-lang#61139.

This PR removes the obsolete printer and replaces all uses of it with `FmtPrinter`. Of the replaced uses, all but one use was in `debug!` logging, two cases were notable:

- `MonoItem::to_string` is used in `-Z print-mono-items` and therefore affects the output of all codegen-units tests (which have been updated).
- `DefPathBasedNames` was used in `librustc_codegen_llvm/type_of.rs` with `LLVMStructCreateNamed` and that'll now get different values, but nothing will break as a result of this.

cc @eddyb (whom I've discussed this with)
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Aug 31, 2020
…te-pretty-printer, r=eddyb

ty: remove obsolete pretty printer

Fixes rust-lang#61139.

This PR removes the obsolete printer and replaces all uses of it with `FmtPrinter`. Of the replaced uses, all but one use was in `debug!` logging, two cases were notable:

- `MonoItem::to_string` is used in `-Z print-mono-items` and therefore affects the output of all codegen-units tests (which have been updated).
- `DefPathBasedNames` was used in `librustc_codegen_llvm/type_of.rs` with `LLVMStructCreateNamed` and that'll now get different values, but nothing will break as a result of this.

cc @eddyb (whom I've discussed this with)
@bors
Copy link
Contributor

bors commented Aug 31, 2020

⌛ Testing commit 6ff471b with merge 8ed5cb5...

@bors
Copy link
Contributor

bors commented Aug 31, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: eddyb
Pushing 8ed5cb5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 31, 2020
@bors bors merged commit 8ed5cb5 into rust-lang:master Aug 31, 2020
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #76027!

Tested on commit 8ed5cb5.
Direct link to PR: #76027

💔 rls on windows: test-pass → build-fail (cc @Xanewok).
💔 rls on linux: test-pass → build-fail (cc @Xanewok).
💔 rustfmt on windows: test-pass → build-fail (cc @topecongiro @calebcartwright).
💔 rustfmt on linux: test-pass → build-fail (cc @topecongiro @calebcartwright).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Aug 31, 2020
Tested on commit rust-lang/rust@8ed5cb5.
Direct link to PR: <rust-lang/rust#76027>

💔 rls on windows: test-pass → build-fail (cc @Xanewok).
💔 rls on linux: test-pass → build-fail (cc @Xanewok).
💔 rustfmt on windows: test-pass → build-fail (cc @topecongiro @calebcartwright).
💔 rustfmt on linux: test-pass → build-fail (cc @topecongiro @calebcartwright).
@davidtwco davidtwco deleted the issue-61139-remove-obsolete-pretty-printer branch August 31, 2020 10:19
@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Sep 1, 2020

This caused a rather large regression that seems to have been fixed in #76030 (results). Overall the net result was a small improvement on optimized and debug builds. Is all of this meeting expectations @davidtwco?

@mati865

This comment has been minimized.

@davidtwco
Copy link
Member Author

This caused a rather large regression that seems to have been fixed in #76030 (results). Overall the net result was a small improvement on optimized and debug builds. Is all of this meeting expectations @davidtwco?

I wasn't expecting quite as large a regression, but it makes sense that the other PR would result in a net improvement.

@ecstatic-morse
Copy link
Contributor

Asked another way, is it possible to get the benefits of the second PR without the regression from this one?

@davidtwco
Copy link
Member Author

davidtwco commented Sep 2, 2020

Asked another way, is it possible to get the benefits of the second PR without the regression from this one?

Yes, you could just revert this.

The second PR adds a early exit which skips the pretty printer for one case (with this PR, it skips the new slower printer, without it skips the old faster printer) - given that this case is most common, it claws most of the performance back. In either case, the impact is really only felt by incremental compilation where the difference is a larger proportion of the overall execution.

I think we’d only see a small improvement from reverting this because the second PR isn’t making this change faster, it’s skipping it and it would be skipping what was there prior to this landing.

@cuviper cuviper added this to the 1.48.0 milestone Nov 17, 2023
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reimplement rustc::ty:print::obsolete with custom PrettyPrinter
8 participants