Skip to content

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
Collaborator

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
Collaborator

bors commented Sep 23, 2017

⌛ Testing commit 3b9e252fb4005c63e5ff0ede06fb5c1c48f1eaa3 with merge ce5534d1854cca6e8f1f8f7bb2a29ba48d8b51ee...

@bors
Copy link
Collaborator

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
Collaborator

bors commented Sep 24, 2017

📌 Commit b6bce56 has been approved by eddyb

@bors
Copy link
Collaborator

bors commented Sep 24, 2017

⌛ Testing commit b6bce56 with merge 001aa73342ff270819b11cb869a32c863ee4f209...

@bors
Copy link
Collaborator

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
Collaborator

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
Collaborator

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