Do not used Move data flow analysis, make it lazy instead #53403

spastorino · 2018-08-15T18:46:01Z

Close #53394

rust-highfive · 2018-08-15T18:46:04Z

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

nikomatsakis · 2018-08-15T18:47:10Z

@bors try

nikomatsakis · 2018-08-15T18:47:15Z

r? @nikomatsakis

bors · 2018-08-15T18:47:21Z

⌛ Trying commit f40cde3 with merge fef1950...

@bors

[WIP] Do not used Move data flow analysis to see perf wins This commit has the only purposed to be tested by: @bors try If this makes sense I'm gonna do the real work :). When all that happens, this should: Close #53394

bors · 2018-08-15T21:19:58Z

☀️ Test successful - status-travis
State: approved= try=True

alexcrichton · 2018-08-15T21:25:47Z

@rust-timer build fef1950

rust-timer · 2018-08-15T23:28:35Z

Success: Queued fef1950 with parent 0f4b498, comparison URL.

BurntPizza · 2018-08-16T05:56:48Z

Perf is ready.

nikomatsakis · 2018-08-17T14:00:11Z

src/librustc_mir/borrow_check/mod.rs

@@ -42,7 +42,7 @@ use dataflow::DataflowResultsConsumer;
 use dataflow::FlowAtLocation;
 use dataflow::MoveDataParamEnv;
 use dataflow::{do_dataflow, DebugFormatted};
-use dataflow::{EverInitializedPlaces, MovingOutStatements};


We should also be able to remove the definition of MovingOutStatements

👍, done!

nikomatsakis · 2018-08-17T14:03:52Z

src/librustc_mir/borrow_check/error_reporting.rs

+            for ii in &self.move_data.init_loc_map[l] {
+                if self.move_data.inits[*ii].path == mpi {
+                    continue 'dfs;
+                }


This should probably be instead:

use rustc_mir::dataflow::drop_flag_effects; let mut any_match = false; drop_flag_effects::for_location_inits( self.tcx, self.mir, self.move_data, l, |m| if m == mpi { any_match = true; }, );

This is a helper function that does the walk for you.

That said, there is a subtle case around call returns which I fear this code may get wrong, but I have to think about the example (in particular, for_local_inits does not walk the initializations due to a call return).

Pushed with your changes to see what happens, but we would need to see the case you're mentioning to see if this code covers that or not.

rust-highfive · 2018-08-18T00:55:32Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
  Downloading https://files.pythonhosted.org/packages/6b/fa/89c248eaacccd816fdea88206060a7cd221f227855782ff7b0ffb80d725a/awscli-1.15.81-py2.py3-none-any.whl (1.3MB)
    0% |▎                               | 10kB 10.4MB/s eta 0:00:01
    1% |▌                               | 20kB 1.8MB/s eta 0:00:01
    2% |▊                               | 30kB 2.2MB/s eta 0:00:01
    3% |█                               | 40kB 2.0MB/s eta 0:00:01
---
    100% |████████████████████████████████| 61kB 6.4MB/s 
Collecting botocore==1.10.80 (from awscli)
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
  Downloading https://files.pythonhosted.org/packages/5e/cf/b97f44993766af17bf64aeddadf66f63b6ebf3d700565cc7ee7b13cd0067/botocore-1.10.80-py2.py3-none-any.whl (4.5MB)
    0% |                                | 10kB 40.8MB/s eta 0:00:01
    0% |▏                               | 20kB 13.1MB/s eta 0:00:01
    0% |▏                               | 30kB 17.5MB/s eta 0:00:01
    0% |▎                               | 40kB 12.1MB/s eta 0:00:01
---
[00:49:06] running 4141 tests
[00:49:08] ....................................................................................................
[00:49:11] ....................................................................................................
[00:49:14] ....................................................................................................
[00:49:17] ........................................F...........................................................
[00:49:24] ........i...........................................................................................
[00:49:29] ..............................................................................................i.....
[00:49:32] ..i.................................................................................................
[00:49:35] ........ii.iii......................................................................................
---
[00:49:56] ....................................................................................................
[00:49:59] ....................................i...............................................................
[00:50:02] ....................................................................................................
[00:50:05] ....................................................................................................
[00:50:08] ...........F........................................................................................
[00:50:15] ....................................................................................................
[00:50:18] ....................................................................................................
[00:50:21] ....................................................................................................
[00:50:25] ....................................................................................................
---
[00:50:59] ....................................................................................................
[00:51:03] .....................................i..............................................................
[00:51:07] ....................................................................................................
[00:51:09] ....................................................................................................
amental to Rust's ownership system: outside\nof workarounds like `Rc`, a value cannot be owned by more than one variable.\n\nSometimes we don't need to move the value. Using a reference, we can let another\nfunction borrow the value without changing its ownership. In the example below,\nwe don't actually have to move our string to `calculate_length`, we can give it\na reference to it with `&` instead.\n\n```\nfn main() {\n    let s1 = String::from(\"hello\");\n\n    let len = calculate_length(&s1);\n\n    println!(\"The length of '{}' is {}.\", s1, len);\n}\n\nfn calculate_length(s: &String) -> usize {\n    s.len()\n}\n```\n\nA mutable reference can be created with `&mut`.\n\nSometimes we don't want a reference, but a duplicate. All types marked `Clone`\ncan be duplicated by calling `.clone()`. Subsequent changes to a clone do not\naffect the original variable.\n\nMost types in the standard library are marked `Clone`. The example below\ndemonstrates using `clone()` on a string. `s1` is first set to \"many\", and then\ncopied to `s2`. Then the first character of `s1` is removed, without affecting\n`s2`. \"any many\" is printed to the console.\n\n```\nfn main() {\n    let mut s1 = String::from(\"many\");\n    let s2 = s1.clone();\n    s1.remove(0);\n    println!(\"{} {}\", s1, s2);\n}\n```\n\nIf we control the definition of a type, we can implement `Clone` on it ourselves\nwith `#[derive(Clone)]`.\n\nSome types have no ownership semantics at all and are trivial to duplicate. An\nexample is `i32` and the other number types. We don't have to call `.clone()` to\nclone them, because they are marked `Copy` in addition to `Clone`.  Implicit\ncloning is more convenient in this case. We can mark our own types `Copy` if\nall their members also are marked `Copy`.\n\nIn the example below, we implement a `Point` type. Because it only stores two\nintegers, we opt-out of ownership semantics with `Copy`. Then we can\n`let p2 = p1` without `p1` being moved.\n\n```\n#[derive(Copy, Clone)]\nstruct Point { x: i32, y: i32 }\n\nfn main() {\n    let mut p1 = Point{ x: -1, y: 2 };\n    let p2 = p1;\n    p1.x = 1;\n    println!(\"p1: {}, {}\", p1.x, p1.y);\n    println!(\"p2: {}, {}\", p2.x, p2.y);\n}\n```\n\nAlternatively, if we don't control the struct's definition, or mutable shared\nownership is truly required, we can use `Rc` and `RefCell`:\n\n```\nuse std::cell::RefCell;\nuse std::rc::Rc;\n\nstruct MyStruct { s: u32 }\n\nfn main() {\n    let mut x = Rc::new(RefCell::new(MyStruct{ s: 5u32 }));\n    let y = x.clone();\n    x.borrow_mut().s = 6;\n    println!(\"{}\", x.borrow().s);\n}\n```\n\nWith this approach, x and y share ownership of the data via the `Rc` (reference\ncount type). `RefCell` essentially performs runtime borrow checking: ensuring\nthat at most one writer or multiple readers can access the data at any one time.\n\nIf you wish to learn more about ownership in Rust, start with the chapter in the\nBook:\n\nhttps://doc.rust-lang.org/book/first-edition/ownership.html\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/borrowck/borrowck-storage-dead.rs","byte_start":764,"byte_end":765,"line_start":30,"line_end":30,"column_start":5,"column_end":6,"is_primary":false,"text":[{"text":"    }","highlight_start":5,"highlight_end":6}],"label":"value moalculate_length(s: &String) -> usize {\n    s.len()\n}\n```\n\nA mutable reference can be created with `&mut`.\n\nSometimes we don't want a reference, but a duplicate. All types marked `Clone`\ncan be duplicated by calling `.clone()`. Subsequent changes to a clone do not\naffect the original variable.\n\nMost types in the standard library are marked `Clone`. The example below\ndemonstrates using `clone()` on a string. `s1` is first set to \"many\", and then\ncopied to `s2`. Then the first character of `s1` is removed, without affecting\n`s2`. \"any many\" is printed to the console.\n\n```\nfn main() {\n    let mut s1 = String::from(\"many\");\n    let s2 = s1.clone();\n    s1.remove(0);\n    println!(\"{} {}\", s1, s2);\n}\n```\n\nIf we control the definition of a type, we can implement `Clone` on it ourselves\nwith `#[derive(Clone)]`.\n\nSome types have no ownership semantics at all and are trivial to duplicate. An\nexample is `i32` and the other number types. We don't have to call `.clone()` to\nclone them, because they are marked `Copy` in addition to `Clone`.  Implicit\ncloning is more convenient in this case. We can mark our own types `Copy` if\nall their members also are marked `Copy`.\n\nIn the example below, we implement a `Point` type. Because it only stores two\nintegers, we opt-out of ownership semantics with `Copy`. Then we can\n`let p2 = p1` without `p1` being moved.\n\n```\n#[derive(Copy, Clone)]\nstruct Point { x: i32, y: i32 }\n\nfn main() {\n    let mut p1 = Point{ x: -1, y: 2 };\n    let p2 = p1;\n    p1.x = 1;\n    println!(\"p1: {}, {}\", p1.x, p1.y);\n    println!(\"p2: {}, {}\", p2.x, p2.y);\n}\n```\n\nAring::String`, which does not implement the `Copy` trait\n\n"}
[00:51:13] {"message":"aborting due to previous error","code":null,"level":"error","spans":[],"children":[],"rendered":"error: aborting due to previous error\n\n"}
[00:51:13] {"message":"For more information about this error, try `rustc --explain E0382`.","code":null,"level":"","spans":[],"children":[],"rendered":"For more information about this error, try `rustc --explain E0382`.\n"}
[00:51:13] ------------------------------------------
[00:51:13] 
[00:51:13] thread '[ui] ui/issues/issue-29723.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3189:9
[00:51:13] 
---
[00:51:13] test result: FAILED. 4119 passed; 2 failed; 20 ignored; 0 measured; 0 filtered out
[00:51:13] 
[00:51:13] 
[00:51:13] 
[00:51:13] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-5.0/bin/FileChecystem      Size  Used Avail Use% Mounted on
tmpfs           1.5G  284K  1.5G   1% /run
/dev/sda1        30G   11G   18G  38% /
none            4.0K     0  4.0K   0% /sys/fs/cgroup
none            5.0M     0  5.0M   0% /run/lock
---
travis_time:end:00d15df8:start=1534553729225081569,finish=1534553729233634890,duration=8553321
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:066ee77c
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

nikomatsakis · 2018-08-24T10:18:09Z

src/librustc_mir/borrow_check/error_reporting.rs

+            stack.push(l);
+        } else {
+            stack.push(Location { block: l.block, statement_index: l.statement_index - 1 });
+        }


As I pointed out on Zulip, this code looks wrong to me. I think you want to add the predecessors_for function that I spelled out there.

nikomatsakis · 2018-08-29T20:41:13Z

@spastorino and I discussed on Zulip. We concluded that the new output is better than the old one, although both could be considered correct. I wanted them to add a comment which I will note here.

nikomatsakis · 2018-08-29T20:41:34Z

src/librustc_mir/borrow_check/error_reporting.rs

+                    if self.move_data.moves[*moi].path == mpi {
+                        debug!("report_use_of_moved_or_uninitialized: found");
+                        result.push(*moi);
+                        continue 'dfs;


Suggested comment:

// Strictly speaking, we could continue our DFS here. There may be // other moves that can reach the point of error. But it is kind of confusing // to highlight them. // // Example: // // ``` // let a = vec![]; // let b = a; // let c = a; // drop(a); // <-- current point of error // ``` // // Because we stop the DFS here, we only highlight `let c = a`, // and not `let b = a`. We will of course also report an error at `let c = a` // which highlights `let b = a` as the move.

nikomatsakis · 2018-08-31T10:08:28Z

@bors r+

bors · 2018-08-31T10:08:29Z

📌 Commit a6aa5dd has been approved by nikomatsakis

bors · 2018-08-31T11:26:02Z

⌛ Testing commit a6aa5dd with merge c2afca3...

Do not used Move data flow analysis, make it lazy instead Close #53394

bors · 2018-08-31T14:06:10Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing c2afca3 to master...

rust-highfive assigned cramertj Aug 15, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 15, 2018

rust-highfive assigned nikomatsakis and unassigned cramertj Aug 15, 2018

rust-lang deleted a comment from rust-timer Aug 15, 2018

spastorino force-pushed the move-out-lazily branch 3 times, most recently from fdbdd51 to 2c2b166 Compare August 16, 2018 23:34

spastorino changed the title ~~[WIP] Do not used Move data flow analysis to see perf wins~~ Do not used Move data flow analysis, make it lazy instead Aug 16, 2018

spastorino force-pushed the move-out-lazily branch from 2c2b166 to 725774a Compare August 17, 2018 04:50

rust-lang deleted a comment from rust-highfive Aug 17, 2018

spastorino force-pushed the move-out-lazily branch from 725774a to ee1b780 Compare August 17, 2018 05:51

rust-lang deleted a comment from rust-highfive Aug 17, 2018

nikomatsakis requested changes Aug 17, 2018

View reviewed changes

spastorino force-pushed the move-out-lazily branch 2 times, most recently from c66e02b to 6f30ed9 Compare August 18, 2018 00:01

rust-lang deleted a comment from rust-highfive Aug 18, 2018

nikomatsakis requested changes Aug 24, 2018

View reviewed changes

nikomatsakis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 24, 2018

memoryruins added the A-NLL Area: Non-lexical lifetimes (NLL) label Aug 28, 2018

spastorino force-pushed the move-out-lazily branch 2 times, most recently from 2060884 to dfaf64a Compare August 28, 2018 19:25

nikomatsakis requested changes Aug 29, 2018

View reviewed changes

spastorino force-pushed the move-out-lazily branch 4 times, most recently from ba06cc7 to 3043310 Compare August 30, 2018 21:56

rust-lang deleted a comment from rust-highfive Aug 30, 2018

spastorino added 2 commits August 30, 2018 19:14

Make move out computation lazy

373fc93

Run rustfmt

a6aa5dd

rust-lang deleted a comment from rust-highfive Aug 30, 2018

spastorino force-pushed the move-out-lazily branch from 3043310 to a6aa5dd Compare August 30, 2018 22:16

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 31, 2018

bors added a commit that referenced this pull request Aug 31, 2018

Auto merge of #53403 - spastorino:move-out-lazily, r=nikomatsakis

c2afca3

Do not used Move data flow analysis, make it lazy instead Close #53394

bors merged commit a6aa5dd into rust-lang:master Aug 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not used Move data flow analysis, make it lazy instead #53403

Do not used Move data flow analysis, make it lazy instead #53403

spastorino commented Aug 15, 2018 •

edited

Loading

rust-highfive commented Aug 15, 2018

nikomatsakis commented Aug 15, 2018

nikomatsakis commented Aug 15, 2018

bors commented Aug 15, 2018

bors commented Aug 15, 2018

alexcrichton commented Aug 15, 2018 •

edited by Mark-Simulacrum

Loading

rust-timer commented Aug 15, 2018

BurntPizza commented Aug 16, 2018

nikomatsakis Aug 17, 2018

spastorino Aug 18, 2018

nikomatsakis Aug 17, 2018

nikomatsakis Aug 17, 2018

spastorino Aug 17, 2018

rust-highfive commented Aug 18, 2018

nikomatsakis Aug 24, 2018

nikomatsakis commented Aug 29, 2018

nikomatsakis Aug 29, 2018

nikomatsakis commented Aug 31, 2018

bors commented Aug 31, 2018

bors commented Aug 31, 2018

bors commented Aug 31, 2018

Do not used Move data flow analysis, make it lazy instead #53403

Do not used Move data flow analysis, make it lazy instead #53403

Conversation

spastorino commented Aug 15, 2018 • edited Loading

rust-highfive commented Aug 15, 2018

nikomatsakis commented Aug 15, 2018

nikomatsakis commented Aug 15, 2018

bors commented Aug 15, 2018

bors commented Aug 15, 2018

alexcrichton commented Aug 15, 2018 • edited by Mark-Simulacrum Loading

rust-timer commented Aug 15, 2018

BurntPizza commented Aug 16, 2018

nikomatsakis Aug 17, 2018

Choose a reason for hiding this comment

spastorino Aug 18, 2018

Choose a reason for hiding this comment

nikomatsakis Aug 17, 2018

Choose a reason for hiding this comment

nikomatsakis Aug 17, 2018

Choose a reason for hiding this comment

spastorino Aug 17, 2018

Choose a reason for hiding this comment

rust-highfive commented Aug 18, 2018

nikomatsakis Aug 24, 2018

Choose a reason for hiding this comment

nikomatsakis commented Aug 29, 2018

nikomatsakis Aug 29, 2018

Choose a reason for hiding this comment

nikomatsakis commented Aug 31, 2018

bors commented Aug 31, 2018

bors commented Aug 31, 2018

bors commented Aug 31, 2018

spastorino commented Aug 15, 2018 •

edited

Loading

alexcrichton commented Aug 15, 2018 •

edited by Mark-Simulacrum

Loading