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

[Draft] Move query definitions into rustc_query_system #78275

Conversation

Julian-Wollersberger
Copy link
Contributor

... and out of rustc_middle, to improve bootstrap times. (cc #65031)

The goal here is to allow future simplifications of the query system, and to make rustc_middle smaller, so that x.py check and x.py build is more parallel.

The important part are the first two commits:
The query definitions in the rustc_queries! proc macro generates several macro_rules macros that are invoked in different parts of rustc_middle. The change here is that one of them, query_description_stream!, now gets all the needed use statements as an input, rather than relying that they are defined at the top of the file. This makes it possible to move the query definitions into rustc_query_system although types like HirId or DefId aren't available there.

The rest of this PR demonstrates one simplification that this schema enables:
DepKind doesn't depend on the query parameter types that are not accessible in rustc_query_system , so one of the macros generated by rustc_queries! can be invoked in rustc_query_system directly to generate the DepKind enum. This enables removing (most of) the DepKind trait and thus removing the generic argument in a bunch of places. (Basically every <D> in #77871.) This doesn't fully work yet, because eg. DepKind::debug needs the global ImplicitTyCxt.

I guess this is an alternative approach to #70951, or maybe a first step to that. (My next todo is reading that PR.) CC rust-lang/compiler-team#277
Can I get your opinions on this approach, @cjgillot and maybe @eddyb and @oli-obk ? Is this a direction worth pursuing?

This PR is more a proof-of-concept and needs cleanup:

  • Update docs to the new location for query definitions
  • do git move on the query definitions file to not lose history
  • rebase on Make fewer types generic over QueryContext #77871
  • make the refactoring of DepKind in a separate PR
  • measure how long rustc_queries! takes, so we know the actual improvement here.

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(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 Oct 23, 2020
@Julian-Wollersberger
Copy link
Contributor Author

@rustbot modify labels: +A-query-system +I-compiletime +T-compiler

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 23, 2020
@Julian-Wollersberger Julian-Wollersberger marked this pull request as draft October 23, 2020 12:34
@jyn514
Copy link
Member

jyn514 commented Oct 23, 2020

do git move on the query definitions file to not lose history

This will have no effect, git move is the same as deleting the old files and running git add on the new ones. GitHub is just bad at following renames.

macro_rules! query_description_stream {
// capture all needed `use` statements.
($($uses:tt)*) => {
$($uses)*
Copy link
Contributor

Choose a reason for hiding this comment

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

You can surround thins expansion by an ad-hoc module to avoid polluting the namespace.

@cjgillot
Copy link
Contributor

FWIW, when I created rustc_query_system, I wanted to have the DepGraph parametered by the DepContext, instead of the DepKind. This required to have DepGraph<TyCtxt<'_>> with a lifetime parameter, which caused lifetime variance problems. The read_deps and with_deps functions in DepKind trait are the middle ground I found at the time.

@Julian-Wollersberger
Copy link
Contributor Author

I currently don't have much time to work on this, so I'll close it for now.
And university starts again next week, so I won't have time in the foreseeable future.

@camelid camelid added the E-help-wanted Call for participation: Help is requested to fix this issue. label Oct 28, 2020
@Julian-Wollersberger
Copy link
Contributor Author

measure how long rustc_queries! takes, so we know the actual improvement here.

FWIW, the proc macro takes 350ms to execute.
(Measured with eprintln!("Query proc macro took {:?}.", end_time.duration_since(start_time));)
The total x.py check time is 2sec less, but that's the variance I normally get.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) E-help-wanted Call for participation: Help is requested to fix this issue. I-compiletime Issue: Problems and improvements with respect to compile times. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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