Optimize the way that loans are killed in borrowck dataflow #51106

davidtwco · 2018-05-27T10:16:51Z

davidtwco · 2018-05-27T10:18:00Z

As of writing this and creating this PR - this pull request builds and I've implemented something that roughly matches how I read the issue and instructions - I don't believe it passes all the tests though and I've not had a chance to look into that yet. I also don't know if it has better performance, not sure what the standard mechanism for checking that is.

Update: Pushed a commit that seems to be fixing some of the errors I was seeing - though I've not had time to run all the tests.

rust-highfive · 2018-05-27T11:23:33Z

The job x86_64-gnu-llvm-3.9 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.

[00:50:08] ....................................................................................................
[00:50:12] ....................................................................................................
[00:50:17] ....................................................................................................
[00:50:23] .....................................................................................i..............
[00:50:28] .............................F................................i.....................................
[00:50:39] ....................................................................................................
[00:50:45] ...........................................................................................i........
------------------------
[00:50:48] 
[00:50:48] 
[00:50:48] ------------------------------------------
[00:50:48] stderr:
[00:50:48] ------------------------------------------
[00:50:48] {"message":"cannot borrow `*map` as mutable because it is also borrowed as immutable (Ast)","code":{"code":"E0502","explanation":"\nThis error indicates that you are trying to borrow a variable as mutable when it\nhas already been borrowed as immutable.\n\nExample of erroneous code:\n\n```compile_fail,E0502\nfn bar(x: &mut i32) {}\nfn foo(a: &mut i32) {\n    let ref y = a; // a is borrowed as immutable.\n    bar(a); // error: cannot borrow `*a` as mutable because `a` is also borrowed\n            //        as immutable\n}\n```\n\nTo fix this error, ensure that you don't have any other references to the\nvariable before trying to access it mutably:\n\n```\nfn bar(x: &mut i32) {}\nfn foo(a: &mut i32) {\n    bar(a);\n    let ref y = a; // ok!\n}\n```\n\nFor more information on the rust ownership system, take a look at\nhttps://doc.rust-lang.org/stable/book/references-and-borrowing.html.\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/nll/get_default.rs","byte_start":1053,"byte_end":1056,"line_start":33,"line_end":33,"column_start":17,"column_end":20,"is_primary":true,"text":[{"text":"                map.set(String::new()); // Ideally, this would not error.","highlight_start":17,"highlight_end":20}],"label":"mutable borrow occurs here","suggested_replacement":null,"suggestion_applicability":null,"expansion":null},{"file_name":"/checkout/src/test/ui/nll/get_default.rs","byte_start":938,"byte_end":941,"line_start":28,"line_end":28,"column_start":15,"column_end":18,"is_primary":false,"text":[{"text":"        match map.get() {","highlight_start":15,"highlight_end":18}],"label":"immutable borrow occurs here","suggested_replacement":null,"suggestion_applicability":null,"expansion":null},{"file_name":"/checkout/src/test/ui/nll/get_default.rs","byte_start":1251,"byte_end":1252,"line_start":39,"line_end":39,"column_start":1,"column_end":2,"is_primary":false,"text":[{"text":"}","highlight_start":1,"highlight_end":2}],"label":"immutable borrow ends here","suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":"error[E0502]: cannot borrow `*map` as mutable because it is also borrowed as immutable (Ast)\n  --> /checkout/src/test/ui/nll/get_default.rs:33:17\n   |\nLL |         match map.get() {\n   |               --- immutable borrow occurs here\n...\nLL |                 map.set(String::new()); // Ideally, this would not error.\n   |                 ^^^ mutable borrow occurs here\n...\nLL | }\n   | - immutable borrow ends here\n\n"}
[00:50:48] {"message":"cannot borrow `*map` as mutable because it is also borrowed as immutable (Ast)","code":{"code":"E0502","explanation":"\nThis error indicates that you are trying to borrow a variable as mutable when it\nhas already been borrowed as immutable.\n\nExample of erroneous code:\n\n```compile_fail,E0502\nfn bar(x: &mut i32) {}\nfn foo(a: &mut i32) {\n    let ref y = a; // a is borrowed as immutable.\n    bar(a); // error: cannot borrow `*a` as mutable because `a` is also borrowed\n            //        as immutable\n}\n```\n\nTo fix this error, ensure that you don't have any other references to the\nvariable before trying to access it mutably:\n\n```\nfn bar(x: &mut i32) {}\nfn foo(a: &mut i32) {\n    bar(a);\n    let ref y = a; // ok!\n}\n```\n\nFor more information on the rust ownership system, take a look at\nhttps://doc.rust-lang.org/stable/book/references-and-borrowing.html.\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/nll/get_default.rs","byte_start":1367,"byte_end":1370,"line_start":45,"line_end":45,"column_start":17,"column_end":20,"is_primary":true,"text":[{"text":"                map.set(String::new()); // Both AST and MIR error here","highlight_start":17,"highlight_end":20}],"label":"mutable borrow occurs here","suggested_replacement":null,"suggestion_applicability":null,"expansion":null},{"file_name":"/checkout/src/test/ui/nll/get_default.rs","byte_start":1314,"byte_end":1317,"line_start":43,"line_end":43,"column_start":15,"column_end":18,"is_primary":false,"text":[{"text":"        match map.get() {","highlight_start":15,"highlight_end":18}],"label":"immutable borrow occurs here","suggested_replacement":null,"suggestion_applicability":null,"expansion":null},{"file_name":"/checkout/src/test/ui/nll/get_default.rs","byte_start":1812,"byte_end":1813,"line_start":57,"line_end":57,"column_start":1,"column_end":2,"is_primary":false,"text":[{"text":"}","highlight_start":1,"highlight_end":2}],"label":"immutable borrow ends here","suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":"error[E0502]: cannot borrow `*map` as mutable because it is also borrowed as immutable (Ast)\n  --> /checkout/src/test/ui/nll/get_default.rs:45:3469736 .
2578316 ./obj/build
1821044 ./obj/build/x86_64-unknown-linux-gnu
727164 ./src
567100 ./obj/build/bootstrap
---
149128 ./src/llvm-emscripten/test
141984 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu
141980 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release
122540 ./obj/build/bootstrap/debug/incremental/bootstrap-1r3bppl29tbrj
122536 ./obj/build/bootstrap/debug/incremental/bootstrap-1r3bppl29tbrj/s-f1f9tgmmr8-zgya24-33jenrrxh1gwm
107128 ./obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release
104172 ./obj/build/x86_64-unknown-linux-gnu/stage0-tools/xfinish=1527420210869139965,duration=8966128
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4

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)

davidtwco · 2018-05-27T21:26:44Z

Pushed up a fix that should stop the DFS getting stuck in cycles.

rust-highfive · 2018-05-27T22:13:52Z

The job x86_64-gnu-llvm-3.9 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.

[00:43:57] ...............................................................i....................................
[00:44:01] ....................................................................................................
[00:44:06] ....................................................................................................
[00:44:13] ............................................................................................i.......
[00:44:15] ..........iiiiiiiii...................................................
[00:44:15] 
[00:44:15] travis_fold:start:test_ui_nll
travis_time:start:test_ui_nll
Check compiletest suite=ui mode=ui compare_mode=nll (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
---
[00:44:56] ......................................................................................i.............
[00:45:01] ...............................................................i....................................
[00:45:05] ....................................................................................................
[00:45:10] ....................................................................................................
[00:45:16] ................F...........................................................................i.......
ould not find any constraint to blame for '_#2r: bb1[0]","code":null,"level":"error: internal compiler error","spans":[],"children":[],"rendered":"error: internal compiler error: librustc_mir/borrow_check/nll/region_infer/mod.rs:1089: could not find any constraint to blame for '_#2r: bb1[0]\n\n"}
[00:45:18] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:45:18] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:45:18] {"message":"aborting due to previous error","code":null,"level":"error","spans":[],"children":[],"rendered":"error: aborting due to previous error\n\n"}
[00:45:18] 
[00:45:18] note: the compiler unexpectedly panicked. this is a bug.
[00:45:18] 
[00:45:18] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[00:45:18] note: rustc 1.28.0-dev running on x86_64-unknown-linux-gnu
[00:45:18] 
[00:45:18] 
[00:45:18] note: compiler flags: -Z ui-testing -Z borrowck=mir -Z two-phase-borrows -Z unstable-options -C prefer-dynamic -C rpath
[00:45:18] 
[00:45:18] ------------------------------------------
[00:45:18] 
[00:45:18] thread '[ui (nll)] ui/span/regions-escape-loop-via-variable.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3053:9

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)

bjorn3 · 2018-05-28T06:25:05Z

src/librustc_mir/dataflow/impls/borrows.rs

+    // If we're not on the last statement, then go to the next
+    // statement in this block.
+    } else {
+        precompute_borrows_out_of_scope(mir, regioncx, borrows_out_of_scope_at_location,


Is llvm able to tail call optimize this?

nikomatsakis · 2018-05-29T08:57:09Z

src/librustc_mir/dataflow/impls/borrows.rs

+        borrows_out_of_scope_at_location
+            .entry(location.clone())
+            .and_modify(|m| m.push(borrow_index))
+            .or_insert(vec![ borrow_index ]);


Wait, you should return here. That is, once the borrow goes out of scope, we kill it, but we can stop the DFS at that point.

Also, wouldn't this normally be

borrows_out_of_scope_at_location .entry(location) .or_insert_with(vec![]) .push(borrow_index);

That seems strictly better... among other things, the formulation as you wrote it here will allocate a vector even when it is not needed.

(Not returning here may be the source of the problem... but I don't see immediately why, it just seems like it would result in vectors that are (much) bigger than necessary.)

Pushed a commit that addresses this.

nikomatsakis · 2018-05-29T09:00:24Z

src/librustc_mir/dataflow/impls/borrows.rs

+    // If we are past the last statement, then check the terminator
+    // to determine which location to proceed to.
+    if location.statement_index == bb_data.statements.len() {
+        if let Some(ref terminator) = bb_data.terminator {


you can invoke the successors method on the terminator instead, would be easier =)

I wish I'd noticed that earlier - fixed.

nikomatsakis · 2018-05-29T09:04:05Z

src/librustc_mir/dataflow/impls/borrows.rs

+    borrow_region: RegionVid,
+    location: Location,
+    visited_locations: &mut Vec<Location>
+) {


Nit: I would definitely write this method with a loop. My usual formulation is something like this:

let mut stack = vec![location]; let mut visited = FxHashSet(); visited.insert(location); while let Some(p) = stack.pop() { process(p); for each n in successors(p) { if visited.insert(n) { stack.push(n); } } }

In this case, process(p) would be something like this:

if !regioncx.region_contains_point(borrow_region, location) { borrows_out_of_scope_at_location .entry(p) .or_insert_with(vec![]) .push(borrow_index); continue; // do not visit successors of this point }

and of course successors(p) is either a call to Terminator::successors or else successor_within_block — not sure if we have a more concise way to get that result right now...

That is much cleaner, not sure what I was thinking when I did it recursively. Fixed this.

…orrow is out of scope at location.

davidtwco · 2018-05-29T17:24:50Z

Pushed up a commit that refactors and addresses previous points. Testing locally shows that this fails even more tests, mostly with the same "could not find any constraint to blame for" error repeated in different tests.

…f a block.

nikomatsakis · 2018-05-29T21:41:20Z

@bors try

bors · 2018-05-29T21:41:32Z

⌛ Trying commit 3a9134d with merge 01aca8254fefbace3ec9fa9de8214cae2f7a07cd...

nikomatsakis · 2018-05-29T21:41:57Z

@Mark-Simulacrum it'd be good to get a perf run on this branch

nikomatsakis · 2018-05-29T21:42:12Z

we expect it to be faster, but I suppose it's possible for it to be slower :)

Mark-Simulacrum · 2018-05-29T21:53:54Z

I've queued the build.. in theory it'll finish before perf gets to it and work smoothly, but if not I'll queue again in a couple hours.

bors · 2018-05-30T00:06:58Z

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

nikomatsakis · 2018-05-30T00:22:59Z

@Mark-Simulacrum ok, try run succeeded now in any case. Approximately how long does a perf run take?

nikomatsakis · 2018-05-30T08:52:00Z

@bors r+

OK, I am not able to get perf to work, as usual. =( This URL doesn't seem to have any data

http://perf.rust-lang.org/compare.html?start=524ad9b9e03656f3fdeb03ed82fe78db3916e566&end=01aca8254fefbace3ec9fa9de8214cae2f7a07cd&stat=instructions%3Au

However, I'm going to r+, because local measurements suggest this is indeed a win.

bors · 2018-05-30T08:52:01Z

📌 Commit 3a9134d has been approved by nikomatsakis

nikomatsakis · 2018-05-30T08:52:45Z

@bors p=1

Giving high priority because getting NLL performant is a 2018 Edition blocker.

bors · 2018-05-30T09:24:28Z

⌛ Testing commit 3a9134d with merge 20af72b...

@nikomatsakis

Optimize the way that loans are killed in borrowck dataflow Fixes #50934. r? @nikomatsakis

bors · 2018-05-30T11:34:55Z

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

Use precomputed DFS of borrows that out of scope at a location.

bbcace5

rust-highfive assigned nikomatsakis May 27, 2018

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

Ensure that we don't skip the last statement.

0eeebe1

Ensure that depth first search does not get stuck in cycles.

24ee506

bjorn3 reviewed May 28, 2018

View reviewed changes

nikomatsakis requested changes May 29, 2018

View reviewed changes

Refactored DFS to be much cleaner. Added continue after noting that b…

4500fe0

…orrow is out of scope at location.

nikomatsakis and others added 4 commits May 29, 2018 14:03

add some debugging statements

9c63714

Ensure that all statements in block are visited not just successors o…

62b1e65

…f a block.

tweak debug output some more

948f77c

fix off by one error

3a9134d

davidtwco changed the title ~~WIP: optimize the way that loans are killed in borrowck dataflow~~ Optimize the way that loans are killed in borrowck dataflow May 29, 2018

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 May 30, 2018

nikomatsakis approved these changes May 30, 2018

View reviewed changes

bors added a commit that referenced this pull request May 30, 2018

Auto merge of #51106 - davidtwco:issue-50934, r=nikomatsakis

20af72b

Optimize the way that loans are killed in borrowck dataflow Fixes #50934. r? @nikomatsakis

bors merged commit 3a9134d into rust-lang:master May 30, 2018

davidtwco deleted the issue-50934 branch May 30, 2018 19:11

Optimize the way that loans are killed in borrowck dataflow #51106

Optimize the way that loans are killed in borrowck dataflow #51106

Uh oh!

Conversation

davidtwco commented May 27, 2018

Uh oh!

davidtwco commented May 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented May 27, 2018

Uh oh!

davidtwco commented May 27, 2018

Uh oh!

rust-highfive commented May 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidtwco commented May 29, 2018

Uh oh!

nikomatsakis commented May 29, 2018

Uh oh!

bors commented May 29, 2018

Uh oh!

nikomatsakis commented May 29, 2018

Uh oh!

nikomatsakis commented May 29, 2018

Uh oh!

Mark-Simulacrum commented May 29, 2018

Uh oh!

bors commented May 30, 2018

Uh oh!

nikomatsakis commented May 30, 2018

Uh oh!

nikomatsakis commented May 30, 2018

Uh oh!

bors commented May 30, 2018

Uh oh!

nikomatsakis commented May 30, 2018

Uh oh!

bors commented May 30, 2018

Uh oh!

bors commented May 30, 2018

Uh oh!

Uh oh!

davidtwco commented May 27, 2018 •

edited

Loading