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] Calculate span information on demand instead of storing it ahead of time #79957

Merged
merged 4 commits into from
Dec 12, 2020
Merged

[rustdoc] Calculate span information on demand instead of storing it ahead of time #79957

merged 4 commits into from
Dec 12, 2020

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Dec 12, 2020 •
edited
Loading

This brings size_of<clean::types::Span>() down from over 100 bytes (!!) to only 12, the same as rustc. It brings Item down even more, from 784 to 680.

TODO: I need to figure out how to do this for the JSON backend too. That uses From impls everywhere, which don't allow passing in the Session as an argument. @P1n3appl3, @tmandry, maybe one of you have ideas? Figured it out, fortunately only two functions needed to be changed. I like the convert_x() format better than From everywhere but I'm open to feedback.

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 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

This comment has been minimized.

Sign in to view
@rust-timer

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@jyn514
Copy link
Member Author

jyn514 commented Dec 12, 2020

This brings stm32h7 to 2425472 KB used (2.42 GB) in 2m 41s, down from 2565508 KB (2.56 GB) in 2m 36s. I think the change in time is just noise from other programs on my system, rustc-perf will have more accurate times.

@jyn514
Copy link
Member Author

jyn514 commented Dec 12, 2020

Not sure when it went from 8.5 GB to 2.5 GB ... maybe I measured wrong at some point?

@jyn514
Copy link
Member Author

jyn514 commented Dec 12, 2020

2611304 (2.6 GB) on beta, 2645148 (2.6 GB) on stable. I think I'm measuring this wrong somehow.

@bors

This comment has been minimized.

Sign in to view
@bors bors 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 Dec 12, 2020
@jyn514

This comment has been minimized.

Sign in to view
@jyn514
Copy link
Member Author

jyn514 commented Dec 12, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Dec 12, 2020

⌛ Trying commit e852bb4bf1a8a460240a01474aaba58221cd9df1 with merge 7a00fa80319b87268f2e011e4d760b5ceabade92...

@jyn514
Copy link
Member Author

jyn514 commented Dec 12, 2020 •
edited
Loading

Ok yes, I was measuring this wrong - the allocation in check_mod_items is allocated and deallocated so quickly that looking at /proc/self/mem with polling misses it. Heaptrack picks up on it though.

Without the changes (39b841d):

bytes allocated in total (ignoring deallocations): 39.31GB (104.62MB/s)
calls to allocation functions: 177032707 (471157/s)
temporary memory allocations: 34929957 (92963/s)
peak heap memory consumption: 2.33GB
peak RSS (including heaptrack overhead): 26.92GB

With the changes:

bytes allocated in total (ignoring deallocations): 39.15GB (112.73MB/s)
calls to allocation functions: 177801926 (512030/s)
temporary memory allocations: 34890948 (100478/s)
peak heap memory consumption: 2.19GB
peak RSS (including heaptrack overhead): 26.08GB

@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 12, 2020
@jyn514 jyn514 marked this pull request as ready for review December 12, 2020 05:04
@bjorn3
Copy link
Member

bjorn3 commented Dec 12, 2020

Rustfmt fails and I think your push canceled the try run.

GuillaumeGomez
GuillaumeGomez approved these changes Dec 12, 2020
View reviewed changes
Copy link
Member

@GuillaumeGomez GuillaumeGomez 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 an awesome PR! Thanks a lot for working on it. I just have one suggestion but you can ignore it if you feel that the current code is better, I have no strong preference here.

@jyn514
Small cleanups
9684557 
- Use a tuple struct instead of a single field
- Enforce calling `source_callsite()` by making the inner span private
- Rename `empty` to `dummy`
@GuillaumeGomez
Copy link
Member

The current code looks good to me. I repeat myself but this is really an amazing PR, thanks a lot! r=me when CI pass. :)

@jyn514
Copy link
Member Author

jyn514 commented Dec 12, 2020

The current code looks good to me. I repeat myself but this is really an amazing PR, thanks a lot! r=me when CI pass. :)

I want to see the perf improvement first, I like seeing green numbers 😆

@jyn514 jyn514 mentioned this pull request Dec 12, 2020
Closed
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (c6c84e4efa0153524a3de2b6c65eac7adf1cc6e7): 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 12, 2020 •
edited
Loading

Up to -2.4% on instructions, up to -3% on max-rss 🎉 🎉

@bors r=GuillaumeGomez

@bors
Copy link
Contributor

bors commented Dec 12, 2020

📌 Commit 9684557 has been approved by GuillaumeGomez

@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 12, 2020
@bors
Copy link
Contributor

bors commented Dec 12, 2020

⌛ Testing commit 9684557 with merge 7efc097...

@jyn514 jyn514 mentioned this pull request Dec 12, 2020
Closed
@bors
Copy link
Contributor

bors commented Dec 12, 2020

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 7efc097 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 12, 2020
@bors bors merged commit 7efc097 into rust-lang:master Dec 12, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 12, 2020
This was referenced Dec 12, 2020
Merged
Merged
Merged
@jyn514 jyn514 deleted the smaller-span branch December 12, 2020 22:48
@jyn514 jyn514 mentioned this pull request Dec 14, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 16, 2020
@bors
Auto merge of rust-lang#80041 - jyn514:shrink-item, r=GuillaumeGomez
90f4b52 
Get rid of `clean::Deprecation`

This brings the size of `item.deprecation` from 56 to 16 bytes. Helps with rust-lang#79103 and rust-lang#76382, in the same vein as rust-lang#79957.

r? `@GuillaumeGomez`
This was referenced Dec 16, 2020
Merged
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 18, 2020
@bors
Auto merge of rust-lang#80090 - jyn514:tcx-in-render, r=GuillaumeGomez
d8d3ab9 
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@GuillaumeGomez GuillaumeGomez

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
Milestone
1.50.0
Development

Successfully merging this pull request may close these issues.
7 participants
@jyn514 @rust-highfive @rust-timer @bors @bjorn3 @GuillaumeGomez @rustbot