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

-Zprint-type-sizes: print the types of awaitees and unnamed coroutine locals. #122922

Merged
merged 1 commit into from
Mar 23, 2024

Conversation

kpreid
Copy link
Contributor

@kpreid kpreid commented Mar 23, 2024

This should assist comprehending the size of coroutines. In particular, whenever a future is suspended while awaiting another future, the latter is given the special name __awaitee, and now the type of the awaited future will be printed, allowing identifying caller/callee — er, I mean, poller/pollee — relationships.

It would be possible to include the type name in more cases, but I thought that that might be overly verbose (print-type-sizes is already a lot of text) and ordinary named fields or variables are easier for readers to discover the types of.

This change will also synergize with my other PR #122923 which changes type printing to print the path of the async fn instead of the span.

Implementation note: I'm not sure if Symbol::intern is appropriate for this application, but it was the obvious way to not have to remove the Copy implementation from FieldInfo, or add a 'tcx lifetime, while avoiding keeping a lot of possibly redundant strings in memory. I don't know what the proper tradeoff to make here is (though presumably it is not too important for a -Z debugging option).

@rustbot
Copy link
Collaborator

rustbot commented Mar 23, 2024

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 23, 2024
… locals.

This should assist comprehending the size of coroutines.
In particular, whenever a future is suspended while awaiting another
future, the latter is given the special name `__awaitee`, and now the
type of the awaited future will be printed, allowing identifying
caller/callee — er, I mean, poller/pollee — relationships.

It would be possible to include the type name in more cases, but I
thought that that might be overly verbose (`print-type-sizes` is already
a lot of text) and ordinary named fields or variables are easier for
readers to discover the types of.
@kpreid kpreid changed the title -Zprint-type-sizes: print coroutine locals' types when they have no name. -Zprint-type-sizes: print the types of awaitees and unnamed coroutine locals. Mar 23, 2024
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Honestly don't see why we can't print this for all fields, tbh. Especially when types are monomorphized, even if you do know the variable it comes from, it's not always clear what the final types end up being.

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 23, 2024

📌 Commit 44d185b has been approved by compiler-errors

It is now in the queue for this repository.

@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 Mar 23, 2024
@compiler-errors
Copy link
Member

Actually wait, just put a Ty<'tcx> into FieldInfo. It implements Display.

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 23, 2024
/// Name of the type of this field.
/// Present only if the creator thought that this would be important for identifying the field,
/// typically because the field name is uninformative.
pub type_name: Option<Symbol>,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub type_name: Option<Symbol>,
pub type_name: Option<Ty<'tcx>>,

Copy link
Contributor Author

@kpreid kpreid Mar 23, 2024

Choose a reason for hiding this comment

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

The Ty we have when constructing FieldInfo is a rustc_middle::ty::Ty. rustc_session, the crate containing FieldInfo, is a dependency of rustc_middle, so that would create a dependency cycle. What do you propose we do about that?

Copy link
Member

Choose a reason for hiding this comment

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

Oh lol

We could use a hook (compiler/rustc_middle/src/hooks/mod.rs) and pull the print_type_sizes + FieldInfo out of rustc_session and down into rustc_ty_utils?

That would have the added benefit of making the layout and the size printing code live together.

Copy link
Member

Choose a reason for hiding this comment

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

Well hm. CodeStats still lives in Session. I wonder if there's anywhere global this can live while also being mutable. Oh well, probably too much work.

Copy link
Member

Choose a reason for hiding this comment

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

global mutable state... 🙀

Copy link
Member

Choose a reason for hiding this comment

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

It's already global mutable state for the record lol

Fixing this would probably just be uplifting it from Session to GlobalCtxt

@compiler-errors
Copy link
Member

Whatever that's too difficult

@bors r+

@bors
Copy link
Contributor

bors commented Mar 23, 2024

📌 Commit 44d185b has been approved by compiler-errors

It is now in the queue for this repository.

@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 Mar 23, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 23, 2024
-Zprint-type-sizes: print the types of awaitees and unnamed coroutine locals.

This should assist comprehending the size of coroutines. In particular, whenever a future is suspended while awaiting another future, the latter is given the special name `__awaitee`, and now the type of the awaited future will be printed, allowing identifying caller/callee — er, I mean, poller/pollee — relationships.

It would be possible to include the type name in more cases, but I thought that that might be overly verbose (`print-type-sizes` is already a lot of text) and ordinary named fields or variables are easier for readers to discover the types of.

This change will also synergize with my other PR rust-lang#122923 which changes type printing to print the path of the `async fn` instead of the span.

Implementation note: I'm not sure if `Symbol::intern` is appropriate for this application, but it was the obvious way to not have to remove the `Copy` implementation from `FieldInfo`, or add a `'tcx` lifetime, while avoiding keeping a lot of possibly redundant strings in memory. I don't know what the proper tradeoff to make here is (though presumably it is not too important for a `-Z` debugging option).
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 23, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#116016 (Soft-destabilize `RustcEncodable` & `RustcDecodable`, remove from prelude in next edition)
 - rust-lang#122460 (Rework rmake support library API)
 - rust-lang#122658 (ci: Build gccjit from a git archive)
 - rust-lang#122698 (Cancel `cargo update` job if there's no updates)
 - rust-lang#122878 (Use `arch::wasm::unreachable` instead of `arch::wasm32::unreachable`)
 - rust-lang#122915 (Delay a bug if no RPITITs were found)
 - rust-lang#122916 (docs(sync): normalize dot in fn summaries)
 - rust-lang#122921 (Enable more mir-opt tests in debug builds)
 - rust-lang#122922 (-Zprint-type-sizes: print the types of awaitees and unnamed coroutine locals.)

r? `@ghost`
`@rustbot` modify labels: rollup
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 23, 2024
-Zprint-type-sizes: print the types of awaitees and unnamed coroutine locals.

This should assist comprehending the size of coroutines. In particular, whenever a future is suspended while awaiting another future, the latter is given the special name `__awaitee`, and now the type of the awaited future will be printed, allowing identifying caller/callee — er, I mean, poller/pollee — relationships.

It would be possible to include the type name in more cases, but I thought that that might be overly verbose (`print-type-sizes` is already a lot of text) and ordinary named fields or variables are easier for readers to discover the types of.

This change will also synergize with my other PR rust-lang#122923 which changes type printing to print the path of the `async fn` instead of the span.

Implementation note: I'm not sure if `Symbol::intern` is appropriate for this application, but it was the obvious way to not have to remove the `Copy` implementation from `FieldInfo`, or add a `'tcx` lifetime, while avoiding keeping a lot of possibly redundant strings in memory. I don't know what the proper tradeoff to make here is (though presumably it is not too important for a `-Z` debugging option).
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 23, 2024
…kingjubilee

Rollup of 10 pull requests

Successful merges:

 - rust-lang#121940 (Mention Register Size in `#[warn(asm_sub_register)]`)
 - rust-lang#122460 (Rework rmake support library API)
 - rust-lang#122698 (Cancel `cargo update` job if there's no updates)
 - rust-lang#122780 (Rename `hir::Local` into `hir::LetStmt`)
 - rust-lang#122875 (CFI: Support self_cell-like recursion)
 - rust-lang#122879 (CFI: Strip auto traits off Virtual calls)
 - rust-lang#122915 (Delay a bug if no RPITITs were found)
 - rust-lang#122916 (docs(sync): normalize dot in fn summaries)
 - rust-lang#122922 (-Zprint-type-sizes: print the types of awaitees and unnamed coroutine locals.)
 - rust-lang#122927 (Change an ICE regression test to use the original reproducer)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 23, 2024
-Zprint-type-sizes: print the types of awaitees and unnamed coroutine locals.

This should assist comprehending the size of coroutines. In particular, whenever a future is suspended while awaiting another future, the latter is given the special name `__awaitee`, and now the type of the awaited future will be printed, allowing identifying caller/callee — er, I mean, poller/pollee — relationships.

It would be possible to include the type name in more cases, but I thought that that might be overly verbose (`print-type-sizes` is already a lot of text) and ordinary named fields or variables are easier for readers to discover the types of.

This change will also synergize with my other PR rust-lang#122923 which changes type printing to print the path of the `async fn` instead of the span.

Implementation note: I'm not sure if `Symbol::intern` is appropriate for this application, but it was the obvious way to not have to remove the `Copy` implementation from `FieldInfo`, or add a `'tcx` lifetime, while avoiding keeping a lot of possibly redundant strings in memory. I don't know what the proper tradeoff to make here is (though presumably it is not too important for a `-Z` debugging option).
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 23, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#122460 (Rework rmake support library API)
 - rust-lang#122698 (Cancel `cargo update` job if there's no updates)
 - rust-lang#122780 (Rename `hir::Local` into `hir::LetStmt`)
 - rust-lang#122875 (CFI: Support self_cell-like recursion)
 - rust-lang#122915 (Delay a bug if no RPITITs were found)
 - rust-lang#122916 (docs(sync): normalize dot in fn summaries)
 - rust-lang#122921 (Enable more mir-opt tests in debug builds)
 - rust-lang#122922 (-Zprint-type-sizes: print the types of awaitees and unnamed coroutine locals.)
 - rust-lang#122927 (Change an ICE regression test to use the original reproducer)

r? `@ghost`
`@rustbot` modify labels: rollup
@kpreid
Copy link
Contributor Author

kpreid commented Mar 23, 2024

Belated note to rollup creator @matthiaskrgr :

#122922 (this PR) and #122923 will conflict with each other in the merge queue — the former adds uses of the printing routine that the latter changes. So whichever one comes second will need re-blessing.

@matthiaskrgr
Copy link
Member

uh oh, thanks!

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 23, 2024
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#120577 (Stabilize slice_split_at_unchecked)
 - rust-lang#122698 (Cancel `cargo update` job if there's no updates)
 - rust-lang#122780 (Rename `hir::Local` into `hir::LetStmt`)
 - rust-lang#122915 (Delay a bug if no RPITITs were found)
 - rust-lang#122916 (docs(sync): normalize dot in fn summaries)
 - rust-lang#122921 (Enable more mir-opt tests in debug builds)
 - rust-lang#122922 (-Zprint-type-sizes: print the types of awaitees and unnamed coroutine locals.)
 - rust-lang#122927 (Change an ICE regression test to use the original reproducer)
 - rust-lang#122930 (add panic location to 'panicked while processing panic')
 - rust-lang#122931 (Fix some typos in the pin.rs)
 - rust-lang#122933 (tag_for_variant follow-ups)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9418f69 into rust-lang:master Mar 23, 2024
11 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 23, 2024
Rollup merge of rust-lang#122922 - kpreid:print-async, r=compiler-errors

-Zprint-type-sizes: print the types of awaitees and unnamed coroutine locals.

This should assist comprehending the size of coroutines. In particular, whenever a future is suspended while awaiting another future, the latter is given the special name `__awaitee`, and now the type of the awaited future will be printed, allowing identifying caller/callee — er, I mean, poller/pollee — relationships.

It would be possible to include the type name in more cases, but I thought that that might be overly verbose (`print-type-sizes` is already a lot of text) and ordinary named fields or variables are easier for readers to discover the types of.

This change will also synergize with my other PR rust-lang#122923 which changes type printing to print the path of the `async fn` instead of the span.

Implementation note: I'm not sure if `Symbol::intern` is appropriate for this application, but it was the obvious way to not have to remove the `Copy` implementation from `FieldInfo`, or add a `'tcx` lifetime, while avoiding keeping a lot of possibly redundant strings in memory. I don't know what the proper tradeoff to make here is (though presumably it is not too important for a `-Z` debugging option).
@kpreid kpreid deleted the print-async branch March 23, 2024 19:50
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 24, 2024
…-errors

In `pretty_print_type()`, print `async fn` futures' paths instead of spans.

This makes `-Zprint-type-sizes`'s output easier to read, because the name of an `async fn` is more immediately recognizable than its span. This change will also synergize with my other `-Zprint-type-sizes` PR rust-lang#122922 which prints the type of child futures being awaited.

I also deleted the comment "FIXME(eddyb) should use `def_span`." because it appears to have already been fixed by commit 67727aa.
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 24, 2024
…-errors

In `pretty_print_type()`, print `async fn` futures' paths instead of spans.

This makes `-Zprint-type-sizes`'s output easier to read, because the name of an `async fn` is more immediately recognizable than its span. This change will also synergize with my other `-Zprint-type-sizes` PR rust-lang#122922 which prints the type of child futures being awaited.

I also deleted the comment "FIXME(eddyb) should use `def_span`." because it appears to have already been fixed by commit 67727aa.
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 24, 2024
…-errors

In `pretty_print_type()`, print `async fn` futures' paths instead of spans.

This makes `-Zprint-type-sizes`'s output easier to read, because the name of an `async fn` is more immediately recognizable than its span. This change will also synergize with my other `-Zprint-type-sizes` PR rust-lang#122922 which prints the type of child futures being awaited.

I also deleted the comment "FIXME(eddyb) should use `def_span`." because it appears to have already been fixed by commit 67727aa.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 24, 2024
…-errors

In `pretty_print_type()`, print `async fn` futures' paths instead of spans.

This makes `-Zprint-type-sizes`'s output easier to read, because the name of an `async fn` is more immediately recognizable than its span. This change will also synergize with my other `-Zprint-type-sizes` PR rust-lang#122922 which prints the type of child futures being awaited.

I also deleted the comment "FIXME(eddyb) should use `def_span`." because it appears to have already been fixed by commit 67727aa.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 25, 2024
…-errors

In `pretty_print_type()`, print `async fn` futures' paths instead of spans.

This makes `-Zprint-type-sizes`'s output easier to read, because the name of an `async fn` is more immediately recognizable than its span. This change will also synergize with my other `-Zprint-type-sizes` PR rust-lang#122922 which prints the type of child futures being awaited.

I also deleted the comment "FIXME(eddyb) should use `def_span`." because it appears to have already been fixed by commit 67727aa.
RenjiSann pushed a commit to RenjiSann/rust that referenced this pull request Mar 25, 2024
-Zprint-type-sizes: print the types of awaitees and unnamed coroutine locals.

This should assist comprehending the size of coroutines. In particular, whenever a future is suspended while awaiting another future, the latter is given the special name `__awaitee`, and now the type of the awaited future will be printed, allowing identifying caller/callee — er, I mean, poller/pollee — relationships.

It would be possible to include the type name in more cases, but I thought that that might be overly verbose (`print-type-sizes` is already a lot of text) and ordinary named fields or variables are easier for readers to discover the types of.

This change will also synergize with my other PR rust-lang#122923 which changes type printing to print the path of the `async fn` instead of the span.

Implementation note: I'm not sure if `Symbol::intern` is appropriate for this application, but it was the obvious way to not have to remove the `Copy` implementation from `FieldInfo`, or add a `'tcx` lifetime, while avoiding keeping a lot of possibly redundant strings in memory. I don't know what the proper tradeoff to make here is (though presumably it is not too important for a `-Z` debugging option).
RenjiSann pushed a commit to RenjiSann/rust that referenced this pull request Mar 25, 2024
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#120577 (Stabilize slice_split_at_unchecked)
 - rust-lang#122698 (Cancel `cargo update` job if there's no updates)
 - rust-lang#122780 (Rename `hir::Local` into `hir::LetStmt`)
 - rust-lang#122915 (Delay a bug if no RPITITs were found)
 - rust-lang#122916 (docs(sync): normalize dot in fn summaries)
 - rust-lang#122921 (Enable more mir-opt tests in debug builds)
 - rust-lang#122922 (-Zprint-type-sizes: print the types of awaitees and unnamed coroutine locals.)
 - rust-lang#122927 (Change an ICE regression test to use the original reproducer)
 - rust-lang#122930 (add panic location to 'panicked while processing panic')
 - rust-lang#122931 (Fix some typos in the pin.rs)
 - rust-lang#122933 (tag_for_variant follow-ups)

r? `@ghost`
`@rustbot` modify labels: rollup
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 25, 2024
…-errors

In `pretty_print_type()`, print `async fn` futures' paths instead of spans.

This makes `-Zprint-type-sizes`'s output easier to read, because the name of an `async fn` is more immediately recognizable than its span. This change will also synergize with my other `-Zprint-type-sizes` PR rust-lang#122922 which prints the type of child futures being awaited.

I also deleted the comment "FIXME(eddyb) should use `def_span`." because it appears to have already been fixed by commit 67727aa.
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 25, 2024
…-errors

In `pretty_print_type()`, print `async fn` futures' paths instead of spans.

This makes `-Zprint-type-sizes`'s output easier to read, because the name of an `async fn` is more immediately recognizable than its span. This change will also synergize with my other `-Zprint-type-sizes` PR rust-lang#122922 which prints the type of child futures being awaited.

I also deleted the comment "FIXME(eddyb) should use `def_span`." because it appears to have already been fixed by commit 67727aa.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 26, 2024
Rollup merge of rust-lang#122923 - kpreid:print-async-def, r=compiler-errors

In `pretty_print_type()`, print `async fn` futures' paths instead of spans.

This makes `-Zprint-type-sizes`'s output easier to read, because the name of an `async fn` is more immediately recognizable than its span. This change will also synergize with my other `-Zprint-type-sizes` PR rust-lang#122922 which prints the type of child futures being awaited.

I also deleted the comment "FIXME(eddyb) should use `def_span`." because it appears to have already been fixed by commit 67727aa.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants