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::Impl to shrink the size of Item #79967

Closed
wants to merge 3 commits into from

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Dec 12, 2020

Helps with #79103. Builds on #79957 and should not be merged before. Eventually I want to calculate this info on demand (#76382) but that turned out to be quite difficult so I'm leaving it for later and slapping on this short-term fix.

This brings the size of Item and ItemKind from

[src/librustdoc/lib.rs:102] std::mem::size_of::<Item>() = 680
[src/librustdoc/lib.rs:102] std::mem::size_of::<ItemKind>() = 408

to

[src/librustdoc/lib.rs:102] std::mem::size_of::<Item>() = 608
[src/librustdoc/lib.rs:102] std::mem::size_of::<ItemKind>() = 336

Waiting to start a perf run until #79957 lands.

@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 12, 2020
@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

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

jyn514 commented Dec 12, 2020

@GuillaumeGomez I plan to box a couple other variants too - would you prefer I do that here or in a separate PR? The only reason I'd separate it is to get more accurate perf numbers, but if we get a perf run on this I think we could compare later runs to the first one?

@GuillaumeGomez
Copy link
Member

I'd prefer in other PRs yes. Like that we can compare more accurately and it makes reviewing easier for me. :)

@jyn514
Copy link
Member Author

jyn514 commented Dec 12, 2020

Another alternative I didn't think of earlier is to box the whole ItemKind. That would reduce the size a lot more without having to box each variant piecemeal.

@jyn514
Copy link
Member Author

jyn514 commented Dec 13, 2020

@GuillaumeGomez I made all the changes I planned to make so that I can compare them to the alternative approach in #80014. If we end up making this change I can split them into separate PRs, but I don't think it make sense to make any of these changes if 80014 gets merged, so I want to compare the final impact.

@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 32b2b42 with merge 7fee63e6e685445eb5cb2757a4441b41db343277...

@bors
Copy link
Contributor

bors commented Dec 13, 2020

☀️ Try build successful - checks-actions
Build commit: 7fee63e6e685445eb5cb2757a4441b41db343277 (7fee63e6e685445eb5cb2757a4441b41db343277)

@rust-timer
Copy link
Collaborator

Queued 7fee63e6e685445eb5cb2757a4441b41db343277 with parent 057937b, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (7fee63e6e685445eb5cb2757a4441b41db343277): 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

@jyn514
Copy link
Member Author

jyn514 commented Dec 14, 2020

Max of -2.1% on instructions, max of -1.3% on max-rss. I think I would prefer to go with #80014 because it improves max-rss more.

@jyn514 jyn514 closed this Dec 14, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 29, 2020
[rustdoc] Box ItemKind to reduce the size of `Item`

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 rust-lang#79967; I don't think it makes sense to make both changes.

Helps with rust-lang#79103.
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. 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.

5 participants