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

rustc_metadata: Load metadata for indirect macro-only dependencies #69432

Merged
merged 1 commit into from
Mar 2, 2020

Conversation

petrochenkov
Copy link
Contributor

Imagine this dependency chain between crates

Executable crate -> Library crate -> Macro crate

where "Library crate" uses the macros from "Macro crate" for some code generation, but doesn't reexport them any further.

Currently, when compiling "Executable crate" we don't even load metadata for it, because why would we want to load any metadata from "Macro crate" if it already did all its code generation job when compiling "Library crate".
Right?

Wrong!
Hygiene data and spans (#68686, #68941) from "Macro crate" still may need to be decoded from "Executable crate".
So we'll have to load them properly.

Questions:

  • How this will affect compile times for larger crate trees in practice? How to measure it?
    Hygiene/span encoding/decoding will necessarily slow down compilation because right now we just don't do some work that we should do, but this introduces a whole new way to slow down things. E.g. loading metadata for syn (and its dependencies) when compiling your executable if one of its library dependencies uses it.
  • We are currently detecting whether a crate reexports macros from "Macro crate" or not, could we similarly detect whether a crate "reexports spans" and keep it unloaded if it doesn't?
    Or at least "reexports important spans" affecting hygiene, we can probably lose spans that only affect diagnostics.

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(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 Feb 24, 2020
@petrochenkov
Copy link
Contributor Author

cc @rust-lang/compiler @ehuss @Aaron1011

@petrochenkov petrochenkov added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 24, 2020
@eddyb
Copy link
Member

eddyb commented Feb 24, 2020

How this will affect compile times for larger crate trees in practice? How to measure it?

Ideally we'd just make it lazy enough for it to be cheap unless the spans are used (e.g. by decoding a mir::Body using Spans from the macro crate).

I'm curious if we have any crates on perf.r-l.o that may be affected:
@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Feb 24, 2020

⌛ Trying commit 487c378 with merge 341097d...

bors added a commit that referenced this pull request Feb 24, 2020
[experiment] rustc_metadata: Load metadata for indirect macro-only dependencies

Imagine this dependency chain between crates
```
Executable crate -> Library crate -> Macro crate
```
where "Library crate" uses the macros from "Macro crate" for some code generation, but doesn't reexport them any further.

Currently, when compiling "Executable crate" we don't even load metadata for it, because why would we want to load any metadata from "Macro crate" if it already did all its code generation job when compiling "Library crate".
Right?

Wrong!
Hygiene data and spans (#68686, #68941) from "Macro crate" still may need to be decoded from "Executable crate".
So we'll have to load them properly.

Questions:
- How this will affect compile times for larger crate trees in practice? How to measure it?
Hygiene/span encoding/decoding will necessarily slow down compilation because right now we just don't do some work that we should do, but this introduces a whole new way to slow down things. E.g. loading metadata for `syn` (and its dependencies) when compiling your executable if one of its library dependencies uses it.
- We are currently detecting whether a crate reexports macros from "Macro crate" or not, could we similarly detect whether a crate "reexports spans" and keep it unloaded if it doesn't?
Or at least "reexports important spans" affecting hygiene, we can probably lose spans that only affect diagnostics.
@bors
Copy link
Contributor

bors commented Feb 24, 2020

☀️ Try build successful - checks-azure
Build commit: 341097d (341097d4377bd629984a3192d2011cb2f21cd4db)

@rust-timer
Copy link
Collaborator

Queued 341097d with parent d9a328a, future comparison URL.

@petrochenkov
Copy link
Contributor Author

The performance diff looks like a noise, but I'm not sure what time is measured by perf.r-l.o.
Is it the target crate only, or the dependencies as well.

Also, why am I even asking this.
The slowdown should affect the target crate as well, even if it's measured separately.
It loads metadata from all its dependency graph, including those new macro dependencies from this PR.

@eddyb
Copy link
Member

eddyb commented Feb 25, 2020

syn-opt is a noise generator, but the rest looks like <0.5% which seems acceptable IMO.

@petrochenkov
Copy link
Contributor Author

Yeah, perhaps this can be just landed then, without further optimizations.

The parts that can be potentially skipped (to what extent?) are fn load and fn register_crate in librustc_metadata/creader.rs which 1) perform file search, 2) load metadata as a binary blob and 3) decode some minimal metadata about the crate root.
AFAIK, those actions were somewhat optimized in the past to speedup zero-change rebuilds.

Copy link
Member

@eddyb eddyb 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 if there are no concerns unrelated to perf left

@matthewjasper matthewjasper assigned eddyb and unassigned matthewjasper Feb 25, 2020
@petrochenkov
Copy link
Contributor Author

(I'll leave this pending for a few days.)

@petrochenkov
Copy link
Contributor Author

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Feb 29, 2020

📌 Commit 487c378 has been approved by eddyb

@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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Feb 29, 2020
@petrochenkov petrochenkov added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 29, 2020
@petrochenkov petrochenkov changed the title [experiment] rustc_metadata: Load metadata for indirect macro-only dependencies rustc_metadata: Load metadata for indirect macro-only dependencies Mar 1, 2020
@bors
Copy link
Contributor

bors commented Mar 2, 2020

⌛ Testing commit 487c378 with merge 9dc8dad...

@bors
Copy link
Contributor

bors commented Mar 2, 2020

☀️ Test successful - checks-azure
Approved by: eddyb
Pushing 9dc8dad to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants