Fix the invalidation of the MIR early exit cache#35751

nagisa · 2016-08-17T14:33:12Z

~~The #34307 introduced a cache for early exits in order to help with O(n*m) explosion of cleanup blocks but the cache is invalidated incorrectly and I can’t seem to figure out why (caching is hard!)~~

~~Remove the cache for now to fix the immediate correctness issue and worry about the performance later.~~

Cache invalidation got fixed.

Fixes #35737

r? @nikomatsakis

eddyb · 2016-08-17T14:40:24Z

I'm not seeing any invalidation at all for cached_exits, seems that it's just missing?

nagisa · 2016-08-17T14:52:25Z

@eddyb it was here

eddyb · 2016-08-17T14:55:43Z

@nagisa Oops, I see. I would've expected .clear() instead of throwing it away.

arielb1 · 2016-08-17T15:45:35Z

@bors r+

bors · 2016-08-17T15:45:37Z

📌 Commit e1749af has been approved by arielb1

arielb1 · 2016-08-17T15:47:37Z

I would prefer to keep the cache through.

nagisa · 2016-08-17T16:05:30Z

Cache invalidation fixed.

eddyb · 2016-08-17T16:08:04Z

src/librustc_mir/build/scope.rs

This will invalidate the unwind cache in cases other than DropKind::Value, which is undesirable (as StorageDead doesn't end up on the unwind path at all).

eddyb · 2016-08-17T17:45:42Z

src/librustc_mir/build/scope.rs

This needs to be &&, not ||.

Fixes rust-lang#35737

eddyb · 2016-08-17T19:31:52Z

@bors r+

bors · 2016-08-17T19:31:53Z

📌 Commit 2d36642 has been approved by eddyb

nikomatsakis · 2016-08-17T20:13:35Z

src/librustc_mir/build/scope.rs

-            if scope.extent == extent {
+            let this_scope = scope.extent == extent;
+            // We must invalidate all the caches leading up to the scope we’re looking for, because
+            // the cached blocks will branch into build of scope not containing the new drop. If we


I have a hard time following this comment, I'm afraid. Ideal would be a diagram, but maybe we can just tweak the wording...I don't quite know how since I don't understand it yet :)

I’ll try to draw something up.

eddyb · 2016-08-17T21:52:38Z

@bors r+

bors · 2016-08-17T21:52:39Z

📌 Commit 2c3250a has been approved by eddyb

@nikomatsakis

Fix the invalidation of the MIR early exit cache ~~The rust-lang#34307 introduced a cache for early exits in order to help with O(n*m) explosion of cleanup blocks but the cache is invalidated incorrectly and I can’t seem to figure out why (caching is hard!)~~ ~~Remove the cache for now to fix the immediate correctness issue and worry about the performance later.~~ Cache invalidation got fixed. Fixes rust-lang#35737 r? @nikomatsakis

Rollup of 12 pull requests - Successful merges: #35346, #35734, #35739, #35740, #35742, #35744, #35749, #35750, #35751, #35756, #35766, #35768 - Failed merges:

nikomatsakis · 2016-08-18T20:20:33Z

Accepting for beta: this was a serious correctness regression.

rust-highfive assigned nikomatsakis Aug 17, 2016

nagisa force-pushed the mir-scope-fix-again branch 2 times, most recently from 85c4e5d to 44955c7 Compare August 17, 2016 16:05

eddyb changed the title ~~Remove the early exit cache~~ Fix the invalidation of the MIR early exit cache Aug 17, 2016

eddyb reviewed Aug 17, 2016
View reviewed changes

eddyb added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 17, 2016

nagisa force-pushed the mir-scope-fix-again branch 3 times, most recently from 112a3e1 to 9983aef Compare August 17, 2016 16:21

eddyb reviewed Aug 17, 2016
View reviewed changes

src/librustc_mir/build/scope.rs Outdated

Copy link

Member

eddyb Aug 17, 2016

Choose a reason for hiding this comment

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

This needs to be &&, not ||.

Properly invalidate the early exit cache

2d36642

Fixes rust-lang#35737

nagisa force-pushed the mir-scope-fix-again branch from 9983aef to 2d36642 Compare August 17, 2016 18:09

nikomatsakis reviewed Aug 17, 2016
View reviewed changes

Nice graphs

2c3250a

nagisa force-pushed the mir-scope-fix-again branch from e08d0cd to 2c3250a Compare August 17, 2016 21:42

eddyb mentioned this pull request Aug 17, 2016

Rollup of 12 pull requests #35769

Merged

bors added a commit that referenced this pull request Aug 18, 2016

Auto merge of #35769 - eddyb:rollup, r=eddyb

9d6520f

Rollup of 12 pull requests - Successful merges: #35346, #35734, #35739, #35740, #35742, #35744, #35749, #35750, #35751, #35756, #35766, #35768 - Failed merges:

bors merged commit 2c3250a into rust-lang:master Aug 18, 2016

nikomatsakis added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Aug 18, 2016

alexcrichton mentioned this pull request Aug 23, 2016

Backport PRs to beta #35914

Merged

brson removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 24, 2016

Uh oh!

Conversation

nagisa commented Aug 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eddyb commented Aug 17, 2016

Uh oh!

nagisa commented Aug 17, 2016

Uh oh!

eddyb commented Aug 17, 2016

Uh oh!

arielb1 commented Aug 17, 2016

Uh oh!

bors commented Aug 17, 2016

Uh oh!

arielb1 commented Aug 17, 2016

Uh oh!

nagisa commented Aug 17, 2016

Uh oh!

eddyb Aug 17, 2016

Choose a reason for hiding this comment

Uh oh!

eddyb Aug 17, 2016

Choose a reason for hiding this comment

Uh oh!

eddyb commented Aug 17, 2016

Uh oh!

bors commented Aug 17, 2016

Uh oh!

nikomatsakis Aug 17, 2016

Choose a reason for hiding this comment

Uh oh!

nagisa Aug 17, 2016

Choose a reason for hiding this comment

Uh oh!

eddyb commented Aug 17, 2016

Uh oh!

bors commented Aug 17, 2016

Uh oh!

nikomatsakis commented Aug 18, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nagisa commented Aug 17, 2016 •

edited

Loading