Skip to content

Encode less metadata for proc-macro crates #76897

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Sep 26, 2020

Conversation

Aaron1011
Copy link
Member

Currently, we serialize the same crate metadata for proc-macro crates as
we do for normal crates. This is quite wasteful - almost none of this
metadata is ever used, and much of it can't even be deserialized (if it
contains a foreign CrateNum).

This PR changes metadata encoding to skip encoding the majority of crate
metadata for proc-macro crates. Most of the Lazy<[T]> fields are left
completetly empty, while the non-lazy fields are left as-is.

Additionally, proc-macros now have a def span that does not include
their body. This was done for normal functions in #75465, but was missed
for proc-macros.

As a result of this PR, we should only ever encode local CrateNums
when encoding proc-macro crates. I've added a specialized serialization
impl for CrateNum to assert this.

@rust-highfive
Copy link
Contributor

r? @petrochenkov

(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 Sep 19, 2020
@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Sep 19, 2020

⌛ Trying commit c0a1bccc2956267914c3f843acf0552cf100025b with merge 8364b62485652df7e6f55f5d5b925082ad5b9094...

@bors
Copy link
Collaborator

bors commented Sep 19, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 8364b62485652df7e6f55f5d5b925082ad5b9094 (8364b62485652df7e6f55f5d5b925082ad5b9094)

@rust-timer
Copy link
Collaborator

Queued 8364b62485652df7e6f55f5d5b925082ad5b9094 with parent bbc6774, future comparison URL.

@jyn514 jyn514 added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-metadata Area: Crate metadata labels Sep 19, 2020
@rust-timer
Copy link
Collaborator

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

@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Sep 19, 2020

⌛ Trying commit 2341efee2714b050e7144d1a6e5c678f4c6bc11e with merge 9d18afcee1a0378a6656c876de82486d528525e7...

@jyn514 jyn514 added the I-compiletime Issue: Problems and improvements with respect to compile times. label Sep 19, 2020
@bors
Copy link
Collaborator

bors commented Sep 19, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 9d18afcee1a0378a6656c876de82486d528525e7 (9d18afcee1a0378a6656c876de82486d528525e7)

@rust-timer
Copy link
Collaborator

Queued 9d18afcee1a0378a6656c876de82486d528525e7 with parent c6ab8e5, future comparison URL.

@rust-timer
Copy link
Collaborator

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

@Aaron1011
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Sep 19, 2020

⌛ Trying commit bc917942284b2721f542ca4348a8706999d2d01a with merge a16f40b1158cae86e56721c06794efa04f3918ce...

@bors
Copy link
Collaborator

bors commented Sep 19, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: a16f40b1158cae86e56721c06794efa04f3918ce (a16f40b1158cae86e56721c06794efa04f3918ce)

@Aaron1011
Copy link
Member Author

cc @rust-lang/infra A perf run didn't get queued for some reason

@Aaron1011
Copy link
Member Author

@rust-timer build a16f40b1158cae86e56721c06794efa04f3918ce

@rust-timer
Copy link
Collaborator

Queued a16f40b1158cae86e56721c06794efa04f3918ce with parent 59fb88d, future comparison URL.

@rust-timer
Copy link
Collaborator

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

@petrochenkov
Copy link
Contributor

r=me with nits addressed.

@petrochenkov petrochenkov 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 Sep 26, 2020
@Aaron1011 Aaron1011 force-pushed the feature/min-proc-macro-metadata branch from bc91794 to be40cd3 Compare September 26, 2020 16:25
Currently, we serialize the same crate metadata for proc-macro crates as
we do for normal crates. This is quite wasteful - almost none of this
metadata is ever used, and much of it can't even be deserialized (if it
contains a foreign `CrateNum`).

This PR changes metadata encoding to skip encoding the majority of crate
metadata for proc-macro crates. Most of the `Lazy<[T]>` fields are left
completetly empty, while the non-lazy fields are left as-is.

Additionally, proc-macros now have a def span that does not include
their body. This was done for normal functions in rust-lang#75465, but was missed
for proc-macros.

As a result of this PR, we should only ever encode local `CrateNum`s
when encoding proc-macro crates. I've added a specialized serialization
impl for `CrateNum` to assert this.
@Aaron1011 Aaron1011 force-pushed the feature/min-proc-macro-metadata branch from be40cd3 to b965356 Compare September 26, 2020 16:26
@Aaron1011
Copy link
Member Author

@bors r=petrochenkov

@bors
Copy link
Collaborator

bors commented Sep 26, 2020

📌 Commit b965356 has been approved by petrochenkov

@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 Sep 26, 2020
@bors
Copy link
Collaborator

bors commented Sep 26, 2020

⌛ Testing commit b965356 with merge 623fb90...

@bors
Copy link
Collaborator

bors commented Sep 26, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: petrochenkov
Pushing 623fb90 to master...

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-metadata Area: Crate metadata 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants