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

record fewer adjustment types in generator witnesses, avoid spurious drops in MIR construction #64584

Merged

Conversation

nikomatsakis
Copy link
Contributor

Don't record all intermediate adjustment types -- That's way more than is needed, and winds up recording types that will never appear in MIR.

Note: I'm like 90% sure that this logic is correct, but this stuff is subtle and can be hard to keep straight. However, the risk of this PR is fairly low -- if we miss types here, I believe the most common outcome is an ICE.

This fixes the original issue cited by #64477, but I'm leaving the issue open for now since there may be other cases we can detect and improve in a targeted way.

r? @Zoxc

That's way more than is needed, and winds up recording types
that will never appear in MIR.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 18, 2019
// traits always return references, which means their content
// can be reborrowed without needing to spill to a temporary.
// If this were not the case, then we could conceivably have
// to create intermediate temporaries.)
Copy link
Contributor

Choose a reason for hiding this comment

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

I ❤️ these comments!

Should probably check how this performs, but it's always nicer to
track just a bit less mutable state.
@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Sep 19, 2019

I added one unrelated commit -- I can factor that out perhaps into a separate PR -- it addresses #64391 and #64433. I decided to leave both in this PR for now because these fixes are fairly high priority and I wanted to save the bors time.

Currently, after a CALL terminator is created in MIR, we insert DROP
statements for all of its operands -- even though they were just moved
shortly before! These spurious drops are later removed, but not before
causing borrow check errors.

This PR series modifies the drop code to track operands that were
moved and avoid creating drops for them.

Right now, I'm only using this mechanism for calls, but it seems
likely it could be used in more places.
@nikomatsakis nikomatsakis force-pushed the issue-64477-generator-capture-types branch from f15278f to b2c51c2 Compare September 19, 2019 15:50
@nikomatsakis
Copy link
Contributor Author

Hmm, so this PR seems fairly urgent -- @eddyb, do you think you'd be able to review?

@nikomatsakis
Copy link
Contributor Author

Mostly I want to land these fixes to avoid problems for async-await.

@eddyb
Copy link
Member

eddyb commented Sep 19, 2019

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned Zoxc Sep 19, 2019
Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

r=me with a better PR description that makes it clear what is (not) being "record"ed

@rust-highfive

This comment has been minimized.

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Sep 19, 2019

welp apparently I broke the test for #40883 somehow (we use more stack now). Not quite sure why that would be. Comparing the MIR between nightly and my branch, it looks basically the same, but I must be missing something.

Edit:

> diff mir_dump/rustc.supersize_me.003-026.PreCodegen.after.mir mir_dump.1/rustc.supersize_me.003-026.PreCodegen.after.mir

huh. Also, when I build and run by hand, I don't see the problem. Hmm.

@nikomatsakis
Copy link
Contributor Author

OK, I think I found the mistake. I didn't intend to screen out StorageDead too.

@nikomatsakis nikomatsakis changed the title don't record all intermediate adjustment types record fewer adjustment types in generator witnesses, avoid spurious drops in MIR construction Sep 19, 2019
@nikomatsakis
Copy link
Contributor Author

So locally I get debuginfo test failures when running this branch, but I am wondering if that is some kind of environmental fail on my end -- I'm hard pressed to imagine how my changes here are causing them, and when I run gdb by hand everything seems to work fine.

@nikomatsakis
Copy link
Contributor Author

@bors r=eddyb p=1

@bors
Copy link
Contributor

bors commented Sep 20, 2019

📌 Commit 77fd0a7 has been approved by eddyb

@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 Sep 20, 2019
@bors
Copy link
Contributor

bors commented Sep 20, 2019

⌛ Testing commit 77fd0a7 with merge 97e58c0...

bors added a commit that referenced this pull request Sep 20, 2019
…es, r=eddyb

record fewer adjustment types in generator witnesses, avoid spurious drops in MIR construction

Don't record all intermediate adjustment types -- That's way more than is needed, and winds up recording types that will never appear in MIR.

Note: I'm like 90% sure that this logic is correct, but this stuff is subtle and can be hard to keep straight.  However, the risk of this PR is fairly low -- if we miss types here, I believe the most common outcome is an ICE.

This fixes the original issue cited by #64477, but I'm leaving the issue open for now since there may be other cases we can detect and improve in a targeted way.

r? @Zoxc
@bors
Copy link
Contributor

bors commented Sep 20, 2019

☀️ Test successful - checks-azure
Approved by: eddyb
Pushing 97e58c0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 20, 2019
@bors bors merged commit 77fd0a7 into rust-lang:master Sep 20, 2019
@nnethercote
Copy link
Contributor

Either this PR or #64498 caused a major regression in rustc perf. It's hard to tell because the graph goes up on #64498 , then down for a single run, then stabilizes again at the worse performance after this PR.

I suggest backing out both PRs, opening new PRs, and then doing perf runs on both. That should make it clear which one cause the regression.

cc @rust-lang/compiler

@Mark-Simulacrum
Copy link
Member

Ah, this might be premature- we recently landed self-profile work on perf which is 95% certainty the cause of the regression, I plan on trying to eliminate those overheads from reported stats in the coming week.

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