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.

Sign up for GitHub

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

Jump to bottom

rustdoc: Compute some fields of clean::Crate on-demand to reduce size #90391

Merged
merged 5 commits into from
Oct 31, 2021
Merged

rustdoc: Compute some fields of clean::Crate on-demand to reduce size #90391

merged 5 commits into from
Oct 31, 2021

Conversation

camelid
Copy link
Member

@camelid camelid commented Oct 29, 2021 •
edited
Loading

clean::Crate is frequently moved by-value -- for example, in DocFolder
implementations -- so reducing its size should improve performance.

This PR reduces the size of clean::Crate from 168 bytes to 104 bytes.

r? @jyn514

@camelid camelid added C-cleanup Category: PRs that clean code up or issues documenting cleanup. I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. I-compiletime Issue: Problems and improvements with respect to compile times. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 29, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 29, 2021
@camelid
Copy link
Member Author

camelid commented Oct 29, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 29, 2021
@bors
Copy link
Contributor

bors commented Oct 29, 2021

⌛ Trying commit a92c191ad66c182bee3f619ececce20fc216c51b with merge a95fcc6c76fa042939aae01abaf13368297003c2...

jyn514
jyn514 reviewed Oct 29, 2021
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

This is great, thanks! Just a couple nits.

src/librustdoc/clean/utils.rs Outdated Show resolved Hide resolved
src/librustdoc/clean/types.rs Outdated Show resolved Hide resolved
@camelid camelid added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 29, 2021
camelid
camelid commented Oct 29, 2021
View reviewed changes
src/librustdoc/clean/types.rs
@@ -115,8 +115,7 @@ impl From<DefId> for ItemId {

#[derive(Clone, Debug)]
crate struct Crate {
crate name: Symbol,
crate src: FileName,
crate num: CrateNum,
crate module: Item,
crate externs: Vec<ExternalCrate>,
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to self: remove this field as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Update: In order to remove this field, Cache::populate will need to take a DocContext, which I think is possible but will require some changes. I'm also working on that as part of #90365, so I'll save removing this field for a future PR.

@bors
Copy link
Contributor

bors commented Oct 29, 2021

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

@rust-timer
Copy link
Collaborator

Queued a95fcc6c76fa042939aae01abaf13368297003c2 with parent df76418, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a95fcc6c76fa042939aae01abaf13368297003c2): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 29, 2021
@jyn514
Copy link
Member

jyn514 commented Oct 29, 2021

Perf says this was actually a regression on max-rss?? That seems very strange, I'm not sure how to interpret it.
Screenshot_20211029-090257

@camelid
Copy link
Member Author

camelid commented Oct 29, 2021

That is really weird. Let's try rerunning perf to see if that was spurious.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 29, 2021
@bors
Copy link
Contributor

bors commented Oct 29, 2021

⌛ Trying commit a92c191ad66c182bee3f619ececce20fc216c51b with merge 73c6d8ebffee3c722b0c6f21fb78c137711a3f3f...

@bors
Copy link
Contributor

bors commented Oct 29, 2021

☀️ Try build successful - checks-actions
Build commit: 73c6d8ebffee3c722b0c6f21fb78c137711a3f3f (73c6d8ebffee3c722b0c6f21fb78c137711a3f3f)

@rust-timer
Copy link
Collaborator

Queued 73c6d8ebffee3c722b0c6f21fb78c137711a3f3f with parent 9ed5b94, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (73c6d8ebffee3c722b0c6f21fb78c137711a3f3f): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 29, 2021
@jyn514
Copy link
Member

jyn514 commented Oct 29, 2021

... apparently that made the RSS regression go away? how strange ...

@camelid
Copy link
Member Author

camelid commented Oct 29, 2021

... apparently that made the RSS regression go away? how strange ...

In my experience, deep-vector often has spurious results, so it's not super surprising. For example, on the most recent perf run above, deep-vector opt had a max-rss regression of 4.85%, which is certainly spurious since this PR did not touch rustc.

@camelid camelid mentioned this pull request Oct 29, 2021
Closed
@jyn514
Copy link
Member

jyn514 commented Oct 30, 2021 •
edited
Loading

@camelid I would rather not add back name, it's cheap to compute and rustdoc should be doing things on-demand.

@camelid
Copy link
Member Author

camelid commented Oct 30, 2021

@camelid I would rather not add back name, it's cheap to compute and rustdoc should be doing things on-demand.

Ok, sounds good. I'll try removing some of the other fields too.

@camelid
Copy link
Member Author

camelid commented Oct 30, 2021

(Currently squashing.)

@camelid camelid removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 30, 2021
@camelid
Copy link
Member Author

camelid commented Oct 30, 2021

Ok, should be good for a new review!

jyn514
jyn514 suggested changes Oct 30, 2021
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

r=me with those nits fixed :)

src/librustdoc/clean/types.rs Outdated Show resolved Hide resolved
src/librustdoc/clean/types.rs Outdated Show resolved Hide resolved
@jyn514 jyn514 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 30, 2021
@camelid
rustdoc: Document that Crate is always local
47b0059
@camelid
rustdoc: Remove Crate.src and instead compute it on-demand
85f8ae8 
It is only used in one place; `src` was about a third of `Crate`'s total
size; `Crate` is frequently moved by-value; and `src` can be easily
computed on-demand.
@camelid
rustdoc: Remove Crate.name and instead compute it on-demand
ebe9a11 
It is not as large as `Crate.src` was, but it's still 8 bytes, and
`clean::Crate` is moved by-value a lot.
@camelid
rustdoc: Stop sorting external crates
a58e214 
Now that rust-lang#73423 is fixed, sorting should no longer be necessary.
See also this discussion [1].

[1]: rust-lang#86889 (comment)
@camelid
Copy link
Member Author

camelid commented Oct 30, 2021

@bors r=jyn514 rollup=never

@bors
Copy link
Contributor

bors commented Oct 30, 2021

📌 Commit a58e214 has been approved by jyn514

@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 Oct 30, 2021
GuillaumeGomez
GuillaumeGomez reviewed Oct 30, 2021
View reviewed changes
src/librustdoc/clean/types.rs
@@ -125,6 +124,20 @@ crate struct Crate {
crate collapsed: bool,
}

// `Crate` is frequently moved by-value. Make sure it doesn't unintentionally get bigger.
#[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
rustc_data_structures::static_assert_size!(Crate, 104);
Copy link
Member

Choose a reason for hiding this comment

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

Nice idea to add this check! 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks :)

@bors
Copy link
Contributor

bors commented Oct 31, 2021

⌛ Testing commit a58e214 with merge 38b01d9...

@bors
Copy link
Contributor

bors commented Oct 31, 2021

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing 38b01d9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 31, 2021
@bors bors merged commit 38b01d9 into rust-lang:master Oct 31, 2021
@rustbot rustbot added this to the 1.58.0 milestone Oct 31, 2021
@bors bors mentioned this pull request Oct 31, 2021
Closed
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (38b01d9): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@camelid camelid deleted the crate-size branch October 31, 2021 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez left review comments

@jyn514 jyn514 jyn514 requested changes

Assignees

@jyn514 jyn514

Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. I-compiletime Issue: Problems and improvements with respect to compile times. 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
Milestone
1.58.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@camelid @rust-timer @bors @jyn514 @GuillaumeGomez @rust-highfive @rustbot