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

Calculate span info on-demand #80339

Merged
merged 4 commits into from
Apr 25, 2021
Merged

Calculate span info on-demand #80339

merged 4 commits into from
Apr 25, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Dec 23, 2020 •
edited
Loading

  • Add helper attr_span for common reused function
  • Stop storing Spans on Item directly; calculate them on demand instead
  • Special case modules, which have different spans depending on whether
    you use inner or outer attributes
  • Special case impls with fake IDs, which can have either dummy spans (for auto traits) or the DefId of the impl block (for blanket impls)
  • Use a fake ID for primitives instead of the ID of the crate; this lets
    source() know that it should use a dummy span instead of the span of
    the crate.

This shrinks Item from 48 to 40 bytes.

Helps with #76382.

@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Dec 23, 2020
@rust-highfive

This comment has been minimized.

Sign in to view
@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 23, 2020
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jyn514
Copy link
Member Author

jyn514 commented Dec 23, 2020

The failure is strange - it looks like the file src/core/slice/ascii.rs.html is no longer being generated? Not sure what that has to do with my changes ... let me try reverting the change to primitive IDs.

@jyn514
Copy link
Member Author

jyn514 commented Dec 23, 2020

let me try reverting the change to primitive IDs.

that did not help :(

@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 Dec 23, 2020
@bors

This comment has been minimized.

Sign in to view
@jyn514 jyn514 mentioned this pull request Dec 31, 2020
Merged
@jyn514 jyn514 force-pushed the no-span branch 2 times, most recently from 5d3d456 to 30d2b24 Compare January 8, 2021 01:15
@jyn514 jyn514 mentioned this pull request Jan 8, 2021
Merged
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 14, 2021
@bors
Auto merge of rust-lang#80802 - jyn514:box-attributes, r=nnethercote
c7b0ddb 
Box Item::Attributes

This reduces the size of Item from 128 to 40 bytes. I think this is as small as it needs to get 🎉

Builds on rust-lang#80339 and should not be merged before.

r? `@GuillaumeGomez`
@bors

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jyn514 jyn514 added I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. I-compiletime Issue: Problems and improvements with respect to compile times. labels Feb 19, 2021
@bors

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (844209ce4023f3d719e4e71382275bb508d23edd): 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 label: +S-waiting-on-review -S-waiting-on-perf

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

jyn514 commented Apr 24, 2021

Up to -1.2% in instruction counts on many of the -doc benchmarks. Max-rss looks like noise.

@bors r=GuillaumeGomez

@bors
Copy link
Contributor

bors commented Apr 24, 2021

📌 Commit 63a08e6d29463892a0dbdb828aa193454e7c79d8 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 Apr 24, 2021
@GuillaumeGomez
Copy link
Member

Less than what I was hoping for but still great!

@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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 24, 2021
@jyn514
Add attr_span helper function
a4b6214
@jyn514
Do the hard part first
b412b46 
The only bit failing was the module, so change that before removing the
`span` field.
@jyn514
Get rid of item.span
c44c64d 
- Remove `span` field, adding `Item::span()` instead
- Special-case `Impl` and `Module` items
- Use dummy spans for primitive items
@jyn514
Finally (finally) ((finally!!!!)) fix spans for the standard library
ba36142
@jyn514
Copy link
Member Author

jyn514 commented Apr 24, 2021

@bors r=GuillaumeGomez

@bors
Copy link
Contributor

bors commented Apr 24, 2021

📌 Commit ba36142 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 24, 2021
@bors
Copy link
Contributor

bors commented Apr 25, 2021

⌛ Testing commit ba36142 with merge f7c468f...

@bors
Copy link
Contributor

bors commented Apr 25, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 25, 2021
@bors bors merged commit f7c468f into rust-lang:master Apr 25, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 25, 2021
This was referenced Apr 25, 2021
Merged
Merged
Merged
@jyn514 jyn514 deleted the no-span branch April 25, 2021 13:43
@camelid camelid mentioned this pull request Jun 11, 2021
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Oct 16, 2023
Closed
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. 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.53.0
Development

Successfully merging this pull request may close these issues.
9 participants
@jyn514 @rust-highfive @rust-log-analyzer @bors @rust-timer @GuillaumeGomez @JohnCSimon @camelid @rustbot