Move the query engine out of rustc_middle #70951
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
for comparison with the measurement from 11days ago: #65031 (comment) |
|
Thank you. This means the query system corresponds to 140s of codegen time. This is huge. I will try to find a way to reduce that. |
|
r? @eddyb |
|
IMO a better approach to our monolithic In crate
cc @rust-lang/compiler if anyone has any thoughts on that approach (it should let us break up the compiler more, and decentralize the queries) As for this PR specifically, I haven't reviewed it yet and I don't know if I'll get to it any time soon, it seems like a pretty complex change. Maybe the new MCP process should be involved? |
|
I agree that the MCP process would be a great fit here! I'm not sure I fully understand what you're proposing, have to read the comment a time or two more, but then again that's kind of the point of the MCP process, isn't it (too encourage enough up-front documentation until people understand what's being proposed). |
I was referring to MCP for this PR, not my suggestion. Although both are MCP-relevant. |
|
I see. Well, I agree with that. =) |
|
I agree with the end goal of decentralizing queries. The point of this PR is to see what needs to stay in librustc_middle, and what does not. From where I stand, the definitions of the |
|
@cjgillot Have you read about the new MCP process (see rust-lang/rfcs#2904)? The idea would be that you can open and issue and summarize the changes made in the PR and the goals with them. This does seem like a large enough change to merit an MCP, although I think it would probably be best to step back a bit and discuss the larger aims (e.g., I too think it'd be good to decentralize the query system. Long term, I would like to extract the query system that the compiler uses into a re-usable library, whether that be salsa or something else, but I don't think we have to start there.) |
|
☔ The latest upstream changes (presumably #70961) made this pull request unmergeable. Please resolve the merge conflicts. |
joelpalmer
added
S-waiting-on-review
|
Ping from Triage: Hi @cjgillot - any updates? |
|
I submitted a MCP at rust-lang/compiler-team#277 |
wesleywiser
added
S-blocked
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #69808) made this pull request unmergeable. Please resolve the merge conflicts. |
jyn514
added
the
I-compiletime
|
📌 Commit 9823c2c has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 9823c2c with merge 3439cb289114f9e715d32f6a854f2cdc74e40688... |
|
💥 Test timed out |
bors
added
S-waiting-on-review
|
@bors retry |
bors
added
S-waiting-on-bors
|
Hanging test: |
|
☀️ Test successful - checks-actions |
|
A job failed! Check out the build log: (web) (plain) Click to see the possible cause of the failure (guessed by this bot) |
@oli-obk @cjgillot Shall we at least open an issue to track this? It would be nice to see if we can gain back the lost performance. The post-merge perf run doesn't seem to point to any obvious culprits though the |
|
@rylev In the post-merge perf run, wall-time, cpu-clock and max-rss are overall green. I am not sure there is a regression to investigate. |
|
The instruction count and wall-time have regressed . We usually investigate when there are regressions in instruction count. |
|
IIUC, some part of the instruction count regression will be attributable to virtual method calls resulting from now invoking queries through the If that's the case, a direct way of addressing it would be to implement devirtualization of applicable trait method calls during LTO, and enable full thin LTO when building rustc. IIUC, devirtualizing should apply here, since there is only one implementor of There is an open issue for full program devirtualization, #68262. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The handling of queries is moved to a trait
QueryEngine.It replaces
query::Queriesin theTyCtxt, allowing to move the query engine out of librustc_middle.There are 2 modes to access the query engine: through
TyCtxtand dynamic dispatch,or through a
QueryCtxt. TheQueryCtxtis required for everything touching theOnDiskCache.For now, I put it in librustc_incremental, which is very small.
This may not be the best place.
A significant part of the codegen time for librustc_middle is moved to the recipient crate.
This PR may require a perf run.
cc #65031
r? @Zoxc