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

Add full debug layout for type layout doc #85862

Closed
wants to merge 1 commit into from

Conversation

pickfire
Copy link
Contributor

@pickfire pickfire commented May 31, 2021

See what you can see for #[rustc_layout(debug)] but with rustdoc which
is very useful, rather than writing some stuff you can just browse through
the docs. I think it may be useful to put it in json and have something
process it and do sort of ncdu to easily optimize on type sizes.

The bad thing is it is at the bottom (I can't find it at first), the good thing is you can go to it with <end>.

image

image

It's hidden by default. Not quite sure how to fix the sticky [+] next to the text. cc @camelid follow up of #83501

It would also be good if we can have a inside rust blog post or something to explain what are the different things within the rust layout.

See what you can see for `#[rustc_layout(debug)]` but with rustdoc which
is very useful, rather than writing some stuff you can just browse through
the docs. I think it may be useful to put it in json and have something
process it and do sort of `ncdu` to easily optimize on type sizes.
@rust-highfive
Copy link
Collaborator

r? @ollie27

(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 May 31, 2021
@pickfire
Copy link
Contributor Author

pickfire commented Jun 1, 2021

I forgot to put the size.

Before

16632   build/x86_64-unknown-linux-gnu/doc/alloc/
32420   build/x86_64-unknown-linux-gnu/doc/book/
10916   build/x86_64-unknown-linux-gnu/doc/cargo/
201192  build/x86_64-unknown-linux-gnu/doc/core/
3200    build/x86_64-unknown-linux-gnu/doc/edition-guide/
6208    build/x86_64-unknown-linux-gnu/doc/embedded-book/
2088    build/x86_64-unknown-linux-gnu/doc/implementors/
6572    build/x86_64-unknown-linux-gnu/doc/nomicon/
812     build/x86_64-unknown-linux-gnu/doc/proc_macro/
11112   build/x86_64-unknown-linux-gnu/doc/reference/
14172   build/x86_64-unknown-linux-gnu/doc/rust-by-example/
6760    build/x86_64-unknown-linux-gnu/doc/rustc/
3452    build/x86_64-unknown-linux-gnu/doc/rustdoc/
64544   build/x86_64-unknown-linux-gnu/doc/src/
85952   build/x86_64-unknown-linux-gnu/doc/std/
1316    build/x86_64-unknown-linux-gnu/doc/test/
49900   build/x86_64-unknown-linux-gnu/doc/unstable-book/

After

16724   build/x86_64-unknown-linux-gnu/doc/alloc/
32420   build/x86_64-unknown-linux-gnu/doc/book/
10916   build/x86_64-unknown-linux-gnu/doc/cargo/
201408  build/x86_64-unknown-linux-gnu/doc/core/
3200    build/x86_64-unknown-linux-gnu/doc/edition-guide/
6208    build/x86_64-unknown-linux-gnu/doc/embedded-book/
2088    build/x86_64-unknown-linux-gnu/doc/implementors/
6572    build/x86_64-unknown-linux-gnu/doc/nomicon/
840     build/x86_64-unknown-linux-gnu/doc/proc_macro/
11112   build/x86_64-unknown-linux-gnu/doc/reference/
14172   build/x86_64-unknown-linux-gnu/doc/rust-by-example/
6760    build/x86_64-unknown-linux-gnu/doc/rustc/
3452    build/x86_64-unknown-linux-gnu/doc/rustdoc/
64544   build/x86_64-unknown-linux-gnu/doc/src/
86300   build/x86_64-unknown-linux-gnu/doc/std/
1392    build/x86_64-unknown-linux-gnu/doc/test/
49900   build/x86_64-unknown-linux-gnu/doc/unstable-book/

Total (non-compressed)

523704 -> 524464 (0.0014490985% increase)

Only std (non-compressed)

85952 -> 86300 (0.0040324450% increase)

@jyn514
Copy link
Member

jyn514 commented Jun 16, 2021

This seems like a misfeature. If you want to see this programmatically you should either use the JSON format or add it to rustc itself. The HTML is explicitly not meant to machine readable.

@pickfire
Copy link
Contributor Author

pickfire commented Jun 16, 2021

This seems like a misfeature. If you want to see this programmatically you should either use the JSON format or add it to rustc itself. The HTML is explicitly not meant to machine readable.

I do plan to add it to JSON output later. But was planning to add it to HTML first. It's meant to be human-readable, the output seemed quite human readable to me, maybe we could improve it a bit by using collapsible tree but this right now I think is good enough as a starting point. Everyone would browse rustdoc using html but not everyone would read the json output which is why I add it to html first.

@jyn514
Copy link
Member

jyn514 commented Jun 16, 2021

The debut output definitely does not look human readable to me, at least for anyone besides a compiler developer. Why would you want this to be specifically in the documentation instead of as a separate option that only shows the layout?

@pickfire
Copy link
Contributor Author

pickfire commented Jun 16, 2021

Why would you want this to be specifically in the documentation instead of as a separate option that only shows the layout?

What do you mean by a separate option that only shows layout?

As to why I would like it to be in rustdoc is that compared to other alternatives, rustdoc is very much used during development. One can easy check API within rustdoc with very low friction. Other approaches that I have seen either (flag) prints too much unfiltered information but only for size or (attr) need to add it one by one recursively which is painful when you have lots of nested data.

In rustdoc you can easy navigate one level up and press the struct/enum and see the size. Other than that, a consumer of the crate can easily identify how space optimized the crate is by just browsing the rustdoc without even pulling the crate, compiling it or adding attributes to it.

@jyn514
Copy link
Member

jyn514 commented Jun 16, 2021

In rustdoc you can easy navigate one level up and press the struct/enum and see the size.

Right, yes, but you can already do that. You're suggesting adding even more debug info about how the layout is generated.

In general "html is nicer than the cli" is not convincing to me because that's an argument for putting everything under the sun in rustdoc. You should give a reason why the additional information makes up for the greater page size and more cluttered UI.

@pickfire
Copy link
Contributor Author

In general "html is nicer than the cli" is not convincing to me because that's an argument for putting everything under the sun in rustdoc. You should give a reason why the additional information makes up for the greater page size and more cluttered UI.

It doesn't clutter the UI since it is collapsed by default and it have negligible impact on the page size. But at the same time we are able to browse through the comprehensive and summarized layout for other crates, it also aids browsing and choosing crates to use, without looking at the source code and downloading the source code and do some modification to check it out.

Yes, giving the full layout there although will not be useful for everyone and every time, but people know it is there and can easily check it out. And yes, I try not to clutter the ui so it does not affect (just <1KB - 0.001% increase total doc size for std) those that does not use this feature, which is why it only occupy a single line from the last page of the screen.

@jyn514
Copy link
Member

jyn514 commented Jun 21, 2021

@jsha @GuillaumeGomez what are your thoughts? I am still not a big fan but I can't articulate why.

@jsha
Copy link
Contributor

jsha commented Jun 21, 2021

I'm also inclined against this, like @jyn514. Here's my reasoning: Public-facing documentation is a contract of sorts. It's meant to tell users of a crate what they can expect from a crate and how to use it. The layout debug information isn't part of that contract. It can't be guaranteed by the crate, because it's not guaranteed by the compiler (as I understand it). And you can also wind up with the odd situation of a crate on docs.rs showing the layout that happened to be in use by the version of nightly it was built with, even if a more recent compiler has changed the layout.

However, part of the reason I'm inclined against it is that I don't really understand the use case. Would you be interested in adding some details about what sort of people you anticipate using this, and what sort of tasks they would get done with it?

@jyn514
Copy link
Member

jyn514 commented Jun 21, 2021

@jsha hmm, by that logic we shouldn't show sizes at all unless they're guaranteed with repr(C). I'm not opposed to that, is it what you meant to suggest?

@pickfire
Copy link
Contributor Author

@jsha There is already the text saying that it's not guaranteed and may change among rust compiler. Even for the size. So not really an issue, user shouldn't expect that it will be the same especially when we have a warning in that part and tons of other ways that tell them that it is just for reference.

But the use case is let users get more information about the layout and internals just for the types, easily look into the optimizations being done by 3rd party crates when auditing. Not all crates have cargo-crev reviews and having more information on these can help the review without digging into the code and types, layers by layers. And not all crates are well documented on the technical implementations, so definitely they can what the compiler says to know how well it was written when comparing. It can also help to guess the size of your struct just by looking at these info or you can even use it to know the exact size of your struct just from your own doc without doing #[rustc_layout(debug)] or --print-type-sizes. I think this might have a bigger impact on rustc development itself since it could be good to easily investigate the niche, I also find the same need to learn more about these details effortlessly.

@jyn514 Maybe we can ask the perf team if they will find these details useful? I wonder what @the8472 thinks if this will be useful in helping him doing perf stuff.

@the8472, by the way congrats on joining the team.

@Mark-Simulacrum
Copy link
Member

I think these debug prints are sometimes useful but I wouldn't really expect to see them in this raw format in the rustdoc view. I think there's definitely possible value in cleaning it up and displaying it nicely alongside the struct definition (i.e., at the top of the HTML), perhaps when --document-private-items or something like it is passed.

But I would probably lean towards "no this is not sufficiently useful" in practice. I think it exposes a lot of low-level details, and no matter the disclaimers about it being an implementation detail will still be "in the docs" and so seemingly OK to rely on. My recollection is that we don't want people doing that for repr(Rust) types even if they've pinned their compiler version -- i.e., the exact layout is an implementation detail within the compiler, and though for reproducible build reasons we generally don't expect to shuffle things too much, it has been proposed that field order be randomly shuffled in debug builds and such (which could affect the total size, too; it's not necessarily a local effect).

I do think some display like this may be helpful eventually for the safe transmute work, which at a high level will give (AIUI) acceptable, stable "views" of the underlying structure of types. Rendering that nicely could be good. It'll always be fairly different from what we hope to accomplish here though.

@pickfire
Copy link
Contributor Author

pickfire commented Jun 21, 2021

I never heard of stable "views". Can you please explain more? My aim is just to expose more information which some (minority) users is able make use of. Yeah, if we want like 90% of people to use it then I think it is not a good idea. But yeah, even if it is useful for 1% of people I think as long as it does not block the way of those 99% people that does not use it it is good enough.

Yes, if there are any suggestions where I could improve this then I will work on it.

@the8472
Copy link
Member

the8472 commented Jun 21, 2021

@jyn514 Maybe we can ask the perf team if they will find these details useful? I wonder what @the8472 thinks if this will be useful in helping him doing perf stuff.

I haven't done much allocation optimization in rust yet, but based on experience in other languages I would prefer the size information for all types in a separate file as a big table to get an overview. What one really needs for optimization is finding the callsites that allocate the most bytes over the lifetime of a program, but that can only be obtained with profiling and not static information.
Anyway, a size breakdown by member would mostly be useful in the case where it's unclear which member bloats up a specific type, which might happen on enums. Basically this level of detail is helpful when drilling down, but we lack the birds-eye perspective before drilling down.

@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jun 22, 2021
@jsha
Copy link
Contributor

jsha commented Jun 22, 2021

@jyn514

by that logic we shouldn't show sizes at all unless they're guaranteed with repr(C). I'm not opposed to that, is it what you meant to suggest?

I didn't realize we already show sizes! Looking now I can see it was added in #83501 and is still behind an unstable flag. Is the idea that docs.rs will enable / has enabled that flag?

Even knowing that we have landed that code, I feel like my reasoning above still stands, and I think it is also a good reason not to make sizes part of the documentation.

Making sizes part of the documentation also notionally breaks encapsulation. If none of a struct's fields are public, users don't really know anything about the representation of that struct, including whether it's big or small, or particularly its exact size. A library author may choose to write "These are large; only make a few of them," or "these are small, make millions of them." The author might even choose to take a small risk and say "these are exactly 24 bytes." But the choice should be up to the author; it's not something we should do on their behalf. As an example of what could go wrong: Someone building an embedded system with small fixed memory could rely on some struct being exactly 24 bytes. When the crate author changes the representation, they would break such a user even though the author wasn't actually making an API break. I realize there are warnings, but I still think it's the wrong tool for the job.

I think @Mark-Simulacrum is on the right track with a dedicated tool. What about this: a standalone tool that can take a crate name as an argument, and prints sizes for all sized types in that crate? This tool could also optionally output the more detailed information from this PR, or could even have flags to select specific aspects to display.

@jyn514
Copy link
Member

jyn514 commented Jun 22, 2021

I didn't realize we already show sizes! Looking now I can see it was added in #83501 and is still behind an unstable flag. Is the idea that docs.rs will enable / has enabled that flag?

I do not think there is a larger plan. I was slightly hesitant about the feature when it was added and I am still hesitant now.

When the crate author changes the representation, they would break such a user even though the author wasn't actually making an API break. I realize there are warnings, but I still think it's the wrong tool for the job.

I think @Mark-Simulacrum is on the right track with a dedicated tool. What about this: a standalone tool that can take a crate name as an argument, and prints sizes for all sized types in that crate? This tool could also optionally output the more detailed information from this PR, or could even have flags to select specific aspects to display.

+1, I'd love to make -Z print-type-sizes more useful instead of making rustdoc's pages bigger.

@pickfire
Copy link
Contributor Author

Anyway, a size breakdown by member would mostly be useful in the case where it's unclear which member bloats up a specific type, which might happen on enums. Basically this level of detail is helpful when drilling down, but we lack the birds-eye perspective before drilling down.

I was thinking of doing the json after this so someone else could build something like ncdu to investigate all the types using sort of cargo type-sizes to investigate the sizes.

I didn't realize we already show sizes! Looking now I can see it was added in #83501 and is still behind an unstable flag. Is the idea that docs.rs will enable / has enabled that flag?

docs.rs didn't enable that flag but rustc doc does https://doc.rust-lang.org/nightly/nightly-rustc/rustc_arena/struct.TypedArena.html (scroll to the bottom)

+1, I'd love to make -Z print-type-sizes more useful instead of making rustdoc's pages bigger.

I didn't look much into that but last time I tried there was some caching issue with that so I stopped using it, at least I didn't go to fix that first.

But still, I think something more useful would be like sort of estimates in compile time or something which works even with Vec to estimate the whole application memory usage but that would be harder than this.

Seemed like no one thinks this is useful, maybe I should just close it?

@jyn514
Copy link
Member

jyn514 commented Jun 22, 2021

But still, I think something more useful would be like sort of estimates in compile time or something which works even with Vec to estimate the whole application memory usage but that would be harder than this.

You cannot measure dynamic memory usage at compile time. You can only do that at runtime with profiling; or at least, finding an estimate at compile time would be a rather tricky research problem and it doesn't belong in rustdoc.

Yes, I think this should be closed.

@jyn514 jyn514 closed this Jun 22, 2021
@pickfire pickfire deleted the rust-layout branch June 22, 2021 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants