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

Use #[rustc_layout] instead of print-type-sizes #69852

Open
RalfJung opened this issue Mar 9, 2020 · 13 comments
Open

Use #[rustc_layout] instead of print-type-sizes #69852

RalfJung opened this issue Mar 9, 2020 · 13 comments
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Mar 9, 2020

@eddyb suggested that the #[rustc_layout] attribute (usage example) should be improved to replace -Z print-type-sizes and then be used for these tests. Then the tests also would not have to be ignore-pass any more.

That attribute also seems very useful when working with LayoutDetails in general in Rust, I would have loved to know about its existence. I wonder what would be a good place to document it?

Also see this blog post on rustc_layout.

@RalfJung RalfJung added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Mar 9, 2020
@Centril Centril added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-testsuite Area: The testsuite used to check the correctness of rustc C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Mar 9, 2020
@eddyb
Copy link
Member

eddyb commented Mar 9, 2020

cc @oli-obk @pnkfelix @nikomatsakis

@wesleywiser
Copy link
Member

I wonder what would be a good place to document it?

We should add a "Compiler attributes" chapter to the Rust Unstable book and include it there.

@nnethercote
Copy link
Contributor

Here is the blog post I wrote 1.5 years ago about -Zprint-type-sizes :) I referred somebody to it just two days ago.

I see two major disadvantages of #[rust_layout] compared to -Zprint-type-sizes.

  • The output of -Zprint-type-sizes is great; it tells me exactly what I need. The output of #[rustc_layout] is more verbose and lower-level; I can barely understand it.
  • #[rustc_layout] only gives you the layout of a single type. What I always want is the layout of a particular type and every type within it. E.g. imagine inspecting the layout of a complex enum like ast::Expr. There are probably 20 or more types that affect its size, and having to add #[rustc_layout] on each of them individually would be a pain.

(I had also been hoping that -Zprint-type-sizes could be made stable, because it's so useful.)

Therefore, #[rustc_layout] is not a sufficient replacement for -Zprint-type-sizes in its current state, and I would strongly object to the removal of -Zprint-type-sizes without major improvements to #[rustc_layout].

@eddyb
Copy link
Member

eddyb commented May 17, 2020

  • The output of -Zprint-type-sizes is great; it tells me exactly what I need. The output of #[rustc_layout] is more verbose and lower-level; I can barely understand it.

This is merely the result of #[rustc_layout(debug)] doing nothing more than {:#?} formatting of a layout, it's not an intentional design point.
-Zprint-type-sizes' output OTOH, while it has some information not found in the layout itself, is lossy in other ways, and the way it tracks the information it needs is pretty ad-hoc and doesn't take advantage of the layout infra (probably because the flag predates that).

The best thing IMO would be an unified output (if we even keep the flag) that is both human-readable and lossless (so that any change would be reflected in layout tests).
Variant and field names would be great, for example (and there are ways to get both of them even for non-ADTs, and we should centralize all that, so we can use it as much as possible).

  • #[rustc_layout] only gives you the layout of a single type. What I always want is the layout of a particular type and every type within it. E.g. imagine inspecting the layout of a complex enum like ast::Expr. There are probably 20 or more types that affect its size, and having to add #[rustc_layout] on each of them individually would be a pain.

I agree, we should have a recursive mode (stopping only at pointers/references), or even make it the default. I could see this being useful for @RalfJung as well.


If we do want to keep the flag, I would at least change "sizes" to "layout" and maybe "print" to "dump". An alternative to stabilization could be moving the flag to info! level logging instead (which we don't guarantee the stability of but should be available even on stable).

It doesn't fit well at all with incremental though, and someone on Discord even pointed out how they wasted time because the flag didn't seem to work until they ran cargo clean.
#[rustc_layout] doesn't have this problem as it always computes the layout and generates the diagnostic, regardless what anything else in the compilation does.

(The conflict with incremental is worse than that theoretically, but since it's an "output (side-)effect", it doesn't really affect soundness, so it's arguably just an odd form of debug logging)


We could even decide to let #[rustc_layout] work on stable/beta (while retaining the error from #![feature(rustc_attrs)] not being available on stable/beta), but I'm not sure how @rust-lang/compiler feels about that approach (or what our default stance on unstable features being observable from stable is, when compilation is guaranteed to not succeed).

@nnethercote
Copy link
Contributor

I agree that -Zprint-type-sizes is a bit clumsy in the way that it prints out every type involved in the compilation. On the other hand, it's nice that -- unlike #[rustc_layout] -- you don't have to modify the code to get the info.

@RalfJung
Copy link
Member Author

I agree, we should have a recursive mode (stopping only at pointers/references), or even make it the default. I could see this being useful for @RalfJung as well.

Yeah that seems reasonable, if we can make it not overwhelming in terms of the sheer amounts of output. {:#?} likely won't cut it any more.

@pickfire
Copy link
Contributor

Should we add some documentation regarding the use of rustc_layout in https://doc.rust-lang.org/unstable-book/language-features/rustc-attrs.html? Before looking at the example I thought we could only use #[rustc_layout(debug)], but after looking at the example here I notice one could use #[rustc_layout(abi, size)].

Also, I found #[rustc_layout(size)] not useful when using it in part of an enum when checking why an enum is so large and the size of each of the elements inside it. I can't seem to do:

enum A {
    #[rustc_layout(size)]
    A(Weird),
}

@RalfJung
Copy link
Member Author

Should we add some documentation regarding the use of rustc_layout in https://doc.rust-lang.org/unstable-book/language-features/rustc-attrs.html?

Yes that seems like a good idea.

Also, I found #[rustc_layout(size)] not useful when using it in part of an enum when checking why an enum is so large and the size of each of the elements inside it. I

Maybe rustc_layout(size) should also print the sizes of all variants?

@pickfire
Copy link
Contributor

Maybe rustc_layout(size) should also print the sizes of all variants?

I don't know if recurseing everything below is a nice idea, one layer seemed good enough for me, not sure about others.

By the way, I used this because -Zprint-type-sizes does not show anything when I recompile, not sure if that is a bug. Both works good for me but I need to run -Zprint-type-sizes for the second time to grep the value or page it since it's too long. This one is more explicit, I got what I wanted but less than what I want (just one size, I want the size of children too).

Maybe this is better? If #[rustc_layout(size)] is used on the enum, show the size of the enum and all the items in the enum. If #[rustc_layout(size)] is used on a member the enum, show the size of the single member. Similarly, do the same for struct. Not sure about union since I never worked with it. If the heuristic like this is simple enough to understand without reading, I think it would be cool.

@nnethercote
Copy link
Contributor

I definitely do want to be able to recurse fully. For a complex type, I often want to see every component of it down to the individual scalars.

@nikomatsakis
Copy link
Contributor

@pickfire I would very much welcome documentation PRs for the unstable book!

@pnkfelix
Copy link
Member

pnkfelix commented Mar 18, 2021

If I understand correctly, there is another problem with relying solely on #[rustc_layout]: You can only use it to get info about types you declare via items.

So, for example, you cannot use it to get the sizes of closures and generators.

  • I was wrong about generators; you can do it unstably as documented by @RalfJung 's blog post like so: playpen

(Of course we could fix this by e.g. extending the attribute further so that you could attach it to fn items, and it would then descend into that fn item to describe the layout of its closures/generator.)

Anyway, my basic viewpoint is that we should have both things. #[rustc_layout] should be available for targeted investigations, while -Z print-type-sizes should be converted to a stable command-line interface and be available for general application profiling purposes.

@eddyb
Copy link
Member

eddyb commented Mar 18, 2021

I was wrong about generators; you can do it unstably

This applies to closures and any other types equally FWIW.

Would be interesting to apply the attribute to a let variable of in a non-generic function, or even just arbitrary expressions.

#[rustc_layout] should be available for targeted investigations, while -Z print-type-sizes should be converted to a stable command-line interface and be available for general application profiling purposes.

I somewhat agree, but to be specific and clear: I think #[rustc_layout] is the better way to test (as in, in the test suite) what layouts we get - and if it's missing any information that -Z print-type-sizes has (such as field names, or even recursing through fields etc.), that should be added.

OTOH "print type sizes" doesn't feel right as a name, but I don't have good suggestions. "report layouts" maybe?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-cleanup Category: PRs that clean code up or issues documenting cleanup. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants