-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Cache eval_to_allocation_raw
on disk
#77006
The head ref may contain hidden characters: "\u{1F40C}_const_queries"
Conversation
@bors try @rust-timer queue |
Awaiting bors try build completion |
⌛ Trying commit c160bf3 with merge 86c0c7bac5afdb8456f7e2d735839fc74aa364f3... |
@@ -716,6 +716,10 @@ rustc_queries! { | |||
"const-evaluating + checking `{}`", | |||
key.value.display(tcx) | |||
} | |||
cache_on_disk_if(_, opt_result) { | |||
// Only store results without errors |
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.
Why do we only want to store results without errors? Or is this a common thing to do?
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.
I think the original motivation was that we can't encode the error, but since today that's just an ErrorHandled
and not the error itself anymore, we can probably start writing the entire thing to disk again.
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.
Hm, @oli-obk did you intend to update this to remove this if here?
I guess we didn't store this to avoid duplication with the other CTFE query, so it it worth now storing that less often? Or do we want the valtree cached as well? |
I think we should measure each individually and act upon those measurements. Computing a valtree may end up being cheap enough for us to never cache it. |
☀️ Try build successful - checks-actions, checks-azure |
Queued 86c0c7bac5afdb8456f7e2d735839fc74aa364f3 with parent a409a23, future comparison URL. |
Finished benchmarking try commit (86c0c7bac5afdb8456f7e2d735839fc74aa364f3): 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 |
Here's an aggregated perf view that shows the diff from before the regression to after this PR: https://perf.rust-lang.org/compare.html?start=10b3595ba6a4c658c9dea105488fc562c815e434&end=86c0c7bac5afdb8456f7e2d735839fc74aa364f3 |
r=me modulo the potential simplification here #77006 (comment) |
@bors r=Mark-Simulacrum |
📌 Commit 40629ef has been approved by |
☀️ Test successful - checks-actions, checks-azure |
#74949 (comment) regressed the performance on these queries, this PR gets the perf back.