-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Misc performance tweaks #57916
Misc performance tweaks #57916
Conversation
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 |
@bors try |
Misc performance tweaks r? @michaelwoerister
☀️ Test successful - checks-travis |
@rust-timer build 0d4be3f |
Success: Queued 0d4be3f with parent 01af120, comparison URL. |
Finished benchmarking try commit 0d4be3f |
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 |
Thanks, @Zoxc! The effects are not big but they all go towards compile time reduction. I'll review some time later this week. |
impl_disk_cacheable_query!(specialization_graph_of, |_| true); | ||
impl_disk_cacheable_query!(mir_borrowck, |tcx, def_id| { | ||
def_id.is_local() && tcx.is_closure(def_id) | ||
}); |
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.
What's the motivation here? If the result is requested (even if it's not used) then the query will be re-run. So unless we make sure that only ensure
is used in all cases that don't need the result, this could backfire. There's at least one case where ensure
is not used here:
rust/src/librustc_driver/driver.rs
Line 1286 in 8a0e5fa
|| tcx.par_body_owners(|def_id| { tcx.mir_borrowck(def_id); })); |
I wonder if this is worth the risk.
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 is just to avoid spending time storing the results of mir_borrowck
for cases where it will never be used 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.
Yes, that makes sense. I wondering though if there's a way of making sure that this doesn't silently break. If somebody adds a non-ensure()
call to mir_borrowck
it would go mostly unnoticed. Well, I guess we would notice on perf.rlo after the fact.
I like the @bors r+ |
📌 Commit ee229f7 has been approved by |
Misc performance tweaks r? @michaelwoerister
☀️ Test successful - checks-travis, status-appveyor |
r? @michaelwoerister