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

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

Merged
merged 8 commits into from May 30, 2018

Conversation

Projects
None yet
6 participants
@davidtwco
Copy link
Member

davidtwco commented May 27, 2018

Fixes #50934.
r? @nikomatsakis

@davidtwco

This comment has been minimized.

Copy link
Member Author

davidtwco commented May 27, 2018

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

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented May 27, 2018

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

This comment has been minimized.

Copy link
Member Author

davidtwco commented May 27, 2018

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

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented May 27, 2018

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)

// 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,

This comment has been minimized.

@bjorn3

bjorn3 May 28, 2018

Contributor

Is llvm able to tail call optimize this?

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

This comment has been minimized.

@nikomatsakis

nikomatsakis May 29, 2018

Contributor

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.

This comment has been minimized.

@nikomatsakis

nikomatsakis May 29, 2018

Contributor

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.

This comment has been minimized.

@nikomatsakis

nikomatsakis May 29, 2018

Contributor

(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.)

This comment has been minimized.

@davidtwco

davidtwco May 29, 2018

Author Member

Pushed a commit that addresses this.

// 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 {

This comment has been minimized.

@nikomatsakis

nikomatsakis May 29, 2018

Contributor

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

This comment has been minimized.

@davidtwco

davidtwco May 29, 2018

Author Member

I wish I'd noticed that earlier - fixed.

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

This comment has been minimized.

@nikomatsakis

nikomatsakis May 29, 2018

Contributor

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...

This comment has been minimized.

@davidtwco

davidtwco May 29, 2018

Author Member

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

@davidtwco

This comment has been minimized.

Copy link
Member Author

davidtwco commented May 29, 2018

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.

nikomatsakis and others added some commits May 29, 2018

@davidtwco 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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented May 29, 2018

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 29, 2018

⌛️ Trying commit 3a9134d with merge 01aca82...

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

Auto merge of #51106 - davidtwco:issue-50934, r=<try>
Optimize the way that loans are killed in borrowck dataflow

Fixes #50934.
r? @nikomatsakis
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented May 29, 2018

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented May 29, 2018

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

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented May 29, 2018

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

This comment has been minimized.

Copy link
Contributor

bors commented May 30, 2018

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented May 30, 2018

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented May 30, 2018

@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

This comment has been minimized.

Copy link
Contributor

bors commented May 30, 2018

📌 Commit 3a9134d has been approved by nikomatsakis

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented May 30, 2018

@bors p=1

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

@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 30, 2018

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

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

Auto merge of #51106 - davidtwco:issue-50934, r=nikomatsakis
Optimize the way that loans are killed in borrowck dataflow

Fixes #50934.
r? @nikomatsakis
@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 30, 2018

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

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

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.