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

Make building THIR a stealable query #85273

Merged
merged 7 commits into from
May 25, 2021
Merged

Conversation

LeSeulArtichaut
Copy link
Contributor

This PR creates a stealable thir_body query so that we can build the THIR only once for THIR unsafeck and MIR build.

Blocked on #83842.
r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 14, 2021
@LeSeulArtichaut LeSeulArtichaut changed the title [WIP] Make building THIR a stealable query Make building THIR a stealable query May 14, 2021
@LeSeulArtichaut
Copy link
Contributor Author

Note: I still need to fix a bug where unsafeck doesn't report all the errors in a few tests; I'll try to work on that tomorrow

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors

This comment has been minimized.

@lqd
Copy link
Member

lqd commented May 19, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 19, 2021
@bors
Copy link
Contributor

bors commented May 19, 2021

⌛ Trying commit abaf435e2379a1e66ab6e58b1daa87042dba28bc with merge 0c06be8f20e88c1e095d9315104aabcf4535af17...

@bors
Copy link
Contributor

bors commented May 19, 2021

☀️ Try build successful - checks-actions
Build commit: 0c06be8f20e88c1e095d9315104aabcf4535af17 (0c06be8f20e88c1e095d9315104aabcf4535af17)

@rust-timer
Copy link
Collaborator

Queued 0c06be8f20e88c1e095d9315104aabcf4535af17 with parent f94942d, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (0c06be8f20e88c1e095d9315104aabcf4535af17): 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 rollup- to bors.

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
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 20, 2021
@nikomatsakis
Copy link
Contributor

So this costs a few percent as is

@rust-log-analyzer

This comment has been minimized.

@LeSeulArtichaut
Copy link
Contributor Author

Ok, so nevermind, building the THIR when a type error occured might not be the best idea. I think I should check for that in thir_body and return an empty THIR?

@LeSeulArtichaut
Copy link
Contributor Author

LeSeulArtichaut commented May 24, 2021

@nikomatsakis I addressed your review comments

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

The code LGTM. I left an idle bit of speculation. I still am a bit confused to see that this seemed to be a performance hit.

@@ -14,6 +14,7 @@ macro_rules! arena_types {
[] layouts: rustc_target::abi::Layout,
// AdtDef are interned and compared by address
[] adt_def: rustc_middle::ty::AdtDef,
[] steal_thir: rustc_data_structures::steal::Steal<rustc_middle::thir::Thir<$tcx>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it would be worth making this a Box<Thir>, so that we don't store as many pointers inline in the map.

@nikomatsakis
Copy link
Contributor

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented May 24, 2021

📌 Commit af3d9a3 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 24, 2021
@bors
Copy link
Contributor

bors commented May 25, 2021

⌛ Testing commit af3d9a3 with merge d568d63...

@bors
Copy link
Contributor

bors commented May 25, 2021

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing d568d63 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 25, 2021
@bors bors merged commit d568d63 into rust-lang:master May 25, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 25, 2021
@LeSeulArtichaut LeSeulArtichaut deleted the thir-query branch May 25, 2021 06:34
@rylev
Copy link
Member

rylev commented May 25, 2021

@nikomatsakis @LeSeulArtichaut It would be really nice to have a little bit more examining why the performance hit was deemed acceptable - maybe 1-2 sentences. I assume this was simply "the feature is desired and the impact is small enough for us to not worry about". Does that jive with your thinking?

@nikomatsakis
Copy link
Contributor

@rylev basically -- however, I wonder if we should have a bit more of a procedure around cases like this. I see that @LeSeulArtichaut opened #85729 which seems to address the problem. I've not caught up on Zulip yet but I imagine somebody may have suggested that, and if we had discussed it more broadly that might've been caught. I guess to start I can ping rust-lang/compiler in the future. However, I'd like it if we had a bit more documentation to help us decide when it's "worth it" (although I guess it will always be a judgement call to some degree).

@nikomatsakis
Copy link
Contributor

I'm thinking something similar to the MCP process-- explain your reasons and give notice when landing a known perf regression and a bit of time for people to respond, but don't require active affirmation.

@nikomatsakis
Copy link
Contributor

I opened up a zulip topic with a proposed perf regression policy along these lines:

https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/proposed.20perf.20regression.20policy

@LeSeulArtichaut
Copy link
Contributor Author

@rylev It looks like the perf regression is due to hashing the whole THIR tree, which seems ineffective even in incr-patched scenarios. I've proposed to not hash the query in #85729, which seems to negate the regression.

If it was not to land for some reason: this is desirable because it allows THIR to be used in different analysis passes but built only once. For now we only use it for MIR building but we will use it for unsafety checking as well (rust-lang/compiler-team#402). We may also be able to shave off some performance by making exhaustiveness checking use the query as well, instead of building its own patterns, though it's not been experimented with yet.

@rylev
Copy link
Member

rylev commented May 27, 2021

Awesome! Thank you all for this! ♥

bors added a commit to rust-lang-ci/rust that referenced this pull request May 27, 2021
…atsakis

Don't hash `thir_body`

Experiment to see if/how much this helps negate the perf impact of rust-lang#85273.
r? `@ghost`
bors added a commit to rust-lang-ci/rust that referenced this pull request May 28, 2021
…s, r=nikomatsakis

Make closures inherit their parent's "safety context"

Fixes rust-lang/project-thir-unsafeck#9, ~~blocked on rust-lang#85273~~.
r? `@nikomatsakis`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants