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

[rustdoc] Box ItemKind to reduce the size of Item #80014

Merged
merged 2 commits into from
Dec 29, 2020

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Dec 13, 2020

This brings the size of Item from

[src/librustdoc/lib.rs:103] std::mem::size_of::<Item>() = 536

to

[src/librustdoc/lib.rs:103] std::mem::size_of::<Item>() = 136

This is an alternative to #79967; I don't think it makes sense to make both changes.

Helps with #79103.

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. labels Dec 13, 2020
@rust-highfive
Copy link
Collaborator

Some changes occurred in intra-doc-links.

cc @jyn514

@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 Dec 13, 2020
@jyn514
Copy link
Member Author

jyn514 commented Dec 13, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Dec 13, 2020

⌛ Trying commit 1b6bb0c79853f4d96b91679f5a01cd6c79291ba8 with merge 5060bdc312fd830ad807a5d08d2f884ef1303a1c...

@jyn514 jyn514 added S-waiting-on-perf Status: Waiting on a perf run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 13, 2020
@bors
Copy link
Contributor

bors commented Dec 13, 2020

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

@rust-timer
Copy link
Collaborator

Queued 5060bdc312fd830ad807a5d08d2f884ef1303a1c with parent 057937b, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (5060bdc312fd830ad807a5d08d2f884ef1303a1c): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

@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 Dec 13, 2020
@jyn514
Copy link
Member Author

jyn514 commented Dec 13, 2020

Up to -1.7% on instructions, up to -3.1% on max-rss. The RSS numbers for rustdoc seem to be less variable than for rustc for some reason.

@jyn514
Copy link
Member Author

jyn514 commented Dec 14, 2020

r? @nnethercote maybe?

@rust-highfive rust-highfive assigned nnethercote and unassigned ollie27 Dec 14, 2020
@pnkfelix
Copy link
Member

Helps with #79103.

Is there a way you could measure how much it helps reduce the peak memory usage there?

@nnethercote
Copy link
Contributor

Can you add a static assertion on the size to prevent future regressions? Something like this, which is a pattern used in various places in the compiler:

// `Item` is used a lot. Make sure it doesn't unintentionally get bigger.
#[cfg(target_arch = "x86_64")]
static_assert_size!(Item, 280);

r=me with that done.

Also, 280 bytes is still large. Most of the AST types in the compiler itself are less than 100 bytes. Which fields are big?

@jyn514
Copy link
Member Author

jyn514 commented Dec 16, 2020

@nnethercote you should have seen when I started, it was 784 bytes 😆 #79957 (comment)

This is all changing rather rapidly so I'm a little hesitant to put an assert since it will make rebasing harder. After rebasing over master the new change is from 616 to 216.

Also, 280 bytes is still large. Most of the AST types in the compiler itself are less than 100 bytes. Which fields are big?

[src/librustdoc/lib.rs:103] std::mem::size_of::<Span>() = 8
[src/librustdoc/lib.rs:103] std::mem::size_of::<Option<rustc_span::Symbol>>() = 4
[src/librustdoc/lib.rs:103] std::mem::size_of::<Attributes>() = 96
[src/librustdoc/lib.rs:103] std::mem::size_of::<Visibility>() = 40
[src/librustdoc/lib.rs:103] std::mem::size_of::<Box<ItemKind>>() = 8
[src/librustdoc/lib.rs:103] std::mem::size_of::<DefId>() = 8
[src/librustdoc/lib.rs:103] std::mem::size_of::<Option<Stability>>() = 16
[src/librustdoc/lib.rs:103] std::mem::size_of::<Option<rustc_attr::Deprecation>>() = 16
[src/librustdoc/lib.rs:103] std::mem::size_of::<Option<ConstStability>>() = 20

So most of the size is coming from Attributes, Visibility, and (Stability, Deprecation, ConstStability). The last three are from rustc directly. A way I thought of to reduce that size is to calculate them on demand with something like

enum ItemId {
    Real(DefId),
    Fake(usize),
}

impl Item {
  fn stability() -> Option<Stability> { ... }
}

and then always return None for fake items. The issue with that is it requires passing a TyCtxt into render, which I have code for, but is a pretty big invasive change.

I do not currently have ideas for reducing the size of Visibility or Attributes.

@jyn514
Copy link
Member Author

jyn514 commented Dec 16, 2020

Helps with #79103.

Is there a way you could measure how much it helps reduce the peak memory usage there?

Measuring this (imprecisely, with time -v, but better than nothing) gives Maximum resident set size (kbytes): 2060060 with this change and Maximum resident set size (kbytes): 2096996 without. So there was an improvement of about 37 MB.

FYI, all of these measurements are done with --features stm32h743, which is the smallest set of features you can add and still compile stm32h7. There are a bunch of others I can add to slow it down.

@bors

This comment has been minimized.

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 18, 2020
Pass a `TyCtxt` through to `FormatRender`

This is the next step after rust-lang#79957 for rust-lang#76382. Eventually I plan to use this to remove `stability`, `const_stability`, and `deprecation` from `Item`, but that needs more extensive changes (in particular, rust-lang#75355 or something like it).

This has no actual changes to behavior, it's just moving types around.

ccc rust-lang#80014 (comment)
@bors

This comment has been minimized.

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 23, 2020
…meGomez

[rustdoc] Calculate stability, const_stability, and deprecation on-demand

Previously, they would always be calculated ahead of time, which bloated the size of `clean::Item`.

Builds on rust-lang#80090 and should not be merged before. Helps with rust-lang#79103 and rust-lang#76382.

cc rust-lang#80014 (comment)

This brings `Item` down to 568 bytes, down from 616.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 23, 2020
…umeGomez

Remove `DefPath` from `Visibility` and calculate it on demand

Depends on rust-lang#80090 and should not be merged before. Helps with rust-lang#79103 and rust-lang#76382.

cc rust-lang#80014 (comment) - `@nnethercote` I figured it out! It was simpler than I expected :)

This brings the size of `clean::Visibility` down from 40 bytes to 8.

Note that this does *not* remove `clean::Visibility`, even though it's now basically the same as `ty::Visibility`, because the `Invsible` variant means something different from `Inherited` and I thought it would be be confusing to merge the two. See the new comments on `impl Clean for ty::Visibility` for details.
@bors

This comment has been minimized.

This brings the size of `Item` from

```
[src/librustdoc/lib.rs:103] std::mem::size_of::<Item>() = 680
```

to

```
[src/librustdoc/lib.rs:103] std::mem::size_of::<Item>() = 280
```
@jyn514
Copy link
Member Author

jyn514 commented Dec 23, 2020

This now brings the size from 536 to 136 bytes.

@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member Author

jyn514 commented Dec 29, 2020

I added the static assertion.

@bors r=nnethercote

@bors
Copy link
Contributor

bors commented Dec 29, 2020

📌 Commit 5fce0dd has been approved by nnethercote

@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 Dec 29, 2020
@jyn514
Copy link
Member Author

jyn514 commented Dec 29, 2020

@bors rollup=never

@bors
Copy link
Contributor

bors commented Dec 29, 2020

⌛ Testing commit 5fce0dd with merge 158f8d0...

@bors
Copy link
Contributor

bors commented Dec 29, 2020

☀️ Test successful - checks-actions
Approved by: nnethercote
Pushing 158f8d0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 29, 2020
@bors bors merged commit 158f8d0 into rust-lang:master Dec 29, 2020
@rustbot rustbot added this to the 1.51.0 milestone Dec 29, 2020
@jyn514 jyn514 deleted the box-item-kind branch December 29, 2020 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. 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
Development

Successfully merging this pull request may close these issues.

9 participants