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

typeck::check::coercion - roll back failed unsizing type vars #44743

Merged
merged 2 commits into from
Sep 24, 2017

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Sep 21, 2017

This wraps unsizing coercions within an additional level of
commit_if_ok, which rolls back type variables if the unsizing coercion
fails. This prevents a large amount of type-variables from accumulating
while type-checking a large function, e.g. shaving 2GB off one of the
4GB peaks in #36799.

This is a performance-sensitive PR so please don't roll it up.

r? @eddyb
cc @nikomatsakis

@eddyb
Copy link
Member

eddyb commented Sep 21, 2017

@bors r+ Nice catch!

@bors
Copy link
Contributor

bors commented Sep 21, 2017

📌 Commit 3b9e252 has been approved by eddyb

@alexcrichton alexcrichton added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 21, 2017
@Mark-Simulacrum
Copy link
Member

@bors p=1 (perf sensitive)

@bors
Copy link
Contributor

bors commented Sep 23, 2017

⌛ Testing commit 3b9e252fb4005c63e5ff0ede06fb5c1c48f1eaa3 with merge ce5534d1854cca6e8f1f8f7bb2a29ba48d8b51ee...

@bors
Copy link
Contributor

bors commented Sep 23, 2017

💔 Test failed - status-appveyor

@kennytm
Copy link
Member

kennytm commented Sep 23, 2017

Error emission order of ui\lifetime-errors\ex3-both-anon-regions-3.rs is changed. Is the test itself flaky? cc @gaurikholkar

 error[E0623]: lifetime mismatch
-  --> $DIR/ex3-both-anon-regions-3.rs:12:13
+  --> $DIR/ex3-both-anon-regions-3.rs:12:15
    |
 11 | fn foo(z: &mut Vec<(&u8,&u8)>, (x, y): (&u8, &u8)) {
-   |                     ---                 --- these two types are declared with different lifetimes...
+   |                         ---                  --- these two types are declared with different lifetimes...
 12 |     z.push((x,y));
-   |             ^ ...but data flows into `z` here
+   |               ^ ...but data flows into `z` here
 
 error[E0623]: lifetime mismatch
-  --> $DIR/ex3-both-anon-regions-3.rs:12:15
+  --> $DIR/ex3-both-anon-regions-3.rs:12:13
    |
 11 | fn foo(z: &mut Vec<(&u8,&u8)>, (x, y): (&u8, &u8)) {
-   |                         ---                  --- these two types are declared with different lifetimes...
+   |                     ---                 --- these two types are declared with different lifetimes...
 12 |     z.push((x,y));
-   |               ^ ...but data flows into `z` here
+   |             ^ ...but data flows into `z` here
 
 error: aborting due to 2 previous errors

@arielb1
Copy link
Contributor Author

arielb1 commented Sep 24, 2017

@kennytm

The order is "flaky" in the sense that it depends on region inference order. Sorting these is non-trivial so I'll just fix up the test.

We'll rewrite the region inference code soon enough anyway.

This wraps unsizing coercions within an additional level of
`commit_if_ok`, which rolls back type variables if the unsizing coercion
fails. This prevents a large amount of type-variables from accumulating
while type-checking a large function, e.g. shaving 2GB off one of the
4GB peaks in rust-lang#36799.
this should produce more error stability
@arielb1
Copy link
Contributor Author

arielb1 commented Sep 24, 2017

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Sep 24, 2017

📌 Commit b6bce56 has been approved by eddyb

@bors
Copy link
Contributor

bors commented Sep 24, 2017

⌛ Testing commit b6bce56 with merge 001aa73342ff270819b11cb869a32c863ee4f209...

@bors
Copy link
Contributor

bors commented Sep 24, 2017

💔 Test failed - status-travis

@arielb1
Copy link
Contributor Author

arielb1 commented Sep 24, 2017

on dist-x86_64-linux:

No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.

@bors retry

@bors
Copy link
Contributor

bors commented Sep 24, 2017

⌛ Testing commit b6bce56 with merge 1ed7d41...

bors added a commit that referenced this pull request Sep 24, 2017
typeck::check::coercion - roll back failed unsizing type vars

This wraps unsizing coercions within an additional level of
`commit_if_ok`, which rolls back type variables if the unsizing coercion
fails. This prevents a large amount of type-variables from accumulating
while type-checking a large function, e.g. shaving 2GB off one of the
4GB peaks in #36799.

This is a performance-sensitive PR so please don't roll it up.

r? @eddyb
cc @nikomatsakis
@bors
Copy link
Contributor

bors commented Sep 24, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 1ed7d41 to master...

@bors bors merged commit b6bce56 into rust-lang:master Sep 24, 2017
bors added a commit that referenced this pull request Sep 25, 2017
encode region::Scope using fewer bytes

Now that region::Scope is no longer interned, its size is more important. This PR encodes region::Scope in 8 bytes instead of 12, which should speed up region inference somewhat (perf testing needed) and should improve the margins on #36799 by 64MB (that's not a lot, I did this PR mostly to speed up region inference).

This is a perf-sensitive PR. Please don't roll me up.

r? @eddyb

This is based on  #44743 so I could get more accurate measurements on #36799.
@alexcrichton
Copy link
Member

This improved compile time of the big-array-of-strings benchmark by 6%

Nice!

@alexcrichton
Copy link
Member

This also decreased memory usage of the same benchmark by 50%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants