-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Make the Query
enum a simple struct.
#80891
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
@bors try @rust-timer queue |
Awaiting bors try build completion. |
⌛ Trying commit e08e9742ba5989cdb6b27f51226311a2e6372d5a with merge dea0e5624588437b1a358664ff4c776fae7d9625... |
☀️ Try build successful - checks-actions |
Queued dea0e5624588437b1a358664ff4c776fae7d9625 with parent a2cd91c, future comparison URL. @rustbot label: +S-waiting-on-perf |
Finished benchmarking try commit (dea0e5624588437b1a358664ff4c776fae7d9625): 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 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 |
@matthewjasper Do you intend to review this or do you want it reassigned? |
This should be reassigned, I'm don't really understand the query system implementation well enough to review this. |
While I review, let's kick off a more recent perf run - it seems like it's worthwhile to see if some of the mixed results (and minor regressions to bootstrap builds) from the previous one have gone away now. @bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit e08e9742ba5989cdb6b27f51226311a2e6372d5a with merge e332f2214ecfdd09201163667071ec79884b208f... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems interesting but at least with the previous performance results not really worth it.
I left some feedback, but it's all mostly minor things and asking for documentation.
I do think it might be worth exploring either reusing or defining a Key trait and storing the Query struct as something roughly like this which might help mitigate some of the performance problems while still giving us a more readable/maintainable implementation.
struct Query<'a> {
name: &'static str,
key: &'a dyn Key,
}
This comment has been minimized.
This comment has been minimized.
The call to `ty::print::with_forced_impl_filename_line` is done when constructing the description, at the construction of the QueryStackFrame.
@bors r+ (Feel free to do this yourself too, if you're just rebasing) |
📌 Commit ea9100f216e2fa276d9fc84dbc1f06c2e81b2bb8 has been approved by |
This comment has been minimized.
This comment has been minimized.
@bors r=Mark-Simulacrum |
📌 Commit 903f65f has been approved by |
⌛ Testing commit 903f65f with merge c32beb4dd2a6d8e1783ba1b66999d2e1757f2ca5... |
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
I can't reproduce the failure. |
☀️ Test successful - checks-actions |
A lot of code in
rustc_query_system
is generic over it, only to encode an exceptional error case: query cycles.The delayed computations are now done at cycle detection.