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

Dont run ast borrowck on mir mode #52083

Merged
merged 2 commits into from Jul 7, 2018

Conversation

Projects
None yet
8 participants
@spastorino
Copy link
Member

spastorino commented Jul 5, 2018

r? @nikomatsakis

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Jul 5, 2018

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 5, 2018

⌛️ Trying commit 48765de with merge b27d9cf...

bors added a commit that referenced this pull request Jul 5, 2018

Auto merge of #52083 - spastorino:dont-run-ast-borrowck-on-mir-mode, …
…r=<try>

Dont run ast borrowck on mir mode

r? @nikomatsakis
@nikomatsakis
Copy link
Contributor

nikomatsakis left a comment

Looks great. One nit.

@@ -89,6 +90,8 @@ pub struct AnalysisData<'a, 'tcx: 'a> {
fn borrowck<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, owner_def_id: DefId)
-> Lrc<BorrowCheckResult>
{
assert!(tcx.borrowck_mode() != BorrowckMode::Mir);

This comment has been minimized.

@nikomatsakis

nikomatsakis Jul 5, 2018

Contributor

Nit: I would do

assert!(match tcx.borrowck_mode() {
    BorrowckMode::Mir => false,
    BorrowckMode::Ast => true,
    // etc
});

i.e., spell out all the cases. That way, when we change the enum (as we plan to soon) we can be sure it is all correct.

@spastorino spastorino force-pushed the spastorino:dont-run-ast-borrowck-on-mir-mode branch from 48765de to 79fb702 Jul 5, 2018

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Jul 5, 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:18:06]    Compiling rustc_passes v0.0.0 (file:///checkout/src/librustc_passes)
[00:18:06] error: unused import: `rustc::session::config::BorrowckMode`
[00:18:06]   --> librustc_borrowck/borrowck/mod.rs:39:5
[00:18:06]    |
[00:18:06] 39 | use rustc::session::config::BorrowckMode;
[00:18:06]    |
[00:18:06]    = note: `-D unused-imports` implied by `-D warnings`
[00:18:06] 
[00:18:07] error: aborting due to previous error
[00:18:07] error: aborting due to previous error
[00:18:07] 
[00:18:07] error: Could not compile `rustc_borrowck`.
[00:18:07] 
[00:18:07] Caused by:
[00:18:07]   process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc --crate-name rustc_borrowck librustc_borrowck/lib.rs --color always --error-format json --crate-type dylib --emit=dep-info,link -C prefer-dynamic -C opt-level=2 -C metadata=b63ed12ca231f4e3 -C extra-filename=-b63ed12ca231f4e3 --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps --target x86_64-unknown-linux-gnu -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/release/deps --extern graphviz=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/libgraphviz-6020508f01da724d.so --extern log=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/liblog-023d781fbd65d983.rlib --extern rustc=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/librustc-1dddb0fa9d8a512f.so --extern rustc_data_structures=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/librustc_data_structures-25582ce13d3618ea.so --extern rustc_errors=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/librustc_errors-e036f8f5b9204e52.so --extern rustc_mir=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/librustc_mir-8fc603afb8ea9b13.so --extern syntax=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/libsyntax-a1b02e0d020520ac.so --extern syntax_pos=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps/libsyntax_pos-f36f6cb494d373be.so -L native=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/build/backtrace-sys-a59a4023b81a89b2/out -L native=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/build/miniz-sys-ea5907e223517cf2/out` (exit code: 101)
[00:18:19] error: build failed
[00:18:19] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" " jemalloc" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json"
[00:18:19] expected success, got: exit code: 101
[00:18:19] expected success, got: exit code: 101
[00:18:19] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1117:9
[00:18:19] travis_fold:end:stage0-rustc

[00:18:19] travis_time:end:stage0-rustc:start=1530815735888662491,finish=1530816541396870522,duration=805508208031


[00:18:19] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:18:19] Build completed unsuccessfully in 0:13:37
[00:18:19] Makefile:28: recipe for target 'all' failed
[00:18:19] make: *** [all] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:01bde1eb
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:0c94e1b0:start=1530816541999796236,finish=1530816542006246005,duration=6449769
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0dee3931
$ head -30 ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
head: cannot open ‘./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers’ for reading: No such file or directory
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:13cfb675
$ dmesg | grep -i kill

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)

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Jul 5, 2018

Try build failed; this needs a fix before perf can go forward.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Jul 5, 2018

Wait, no, I misinterpreted the log. Ignore previous message.

@spastorino spastorino force-pushed the spastorino:dont-run-ast-borrowck-on-mir-mode branch from 79fb702 to 25266c1 Jul 5, 2018

@Mark-Simulacrum

This comment was marked as resolved.

Copy link
Member

Mark-Simulacrum commented Jul 5, 2018

Trying out the new bot:

@rust-timer build b27d9cf

@rust-timer

This comment was marked as resolved.

Copy link

rust-timer commented Jul 5, 2018

Insufficient permissions to issue commands to rust-timer.

@Mark-Simulacrum

This comment was marked as resolved.

Copy link
Member

Mark-Simulacrum commented Jul 5, 2018

@rust-timer

This comment was marked as resolved.

Copy link

rust-timer commented Jul 5, 2018

Insufficient permissions to issue commands to rust-timer.

@Mark-Simulacrum

This comment was marked as resolved.

Copy link
Member

Mark-Simulacrum commented Jul 5, 2018

@rust-timer

This comment was marked as resolved.

Copy link

rust-timer commented Jul 5, 2018

Please provide the full 40 character commit hash.

@Mark-Simulacrum

This comment was marked as resolved.

Copy link
Member

Mark-Simulacrum commented Jul 5, 2018

@rust-timer

This comment was marked as resolved.

Copy link

rust-timer commented Jul 5, 2018

Bors try commit b27d9cf unexpectedly has 2 parents.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Jul 5, 2018

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Jul 5, 2018

Success: Queued b27d9cf with parent 94eb176, comparison URL.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jul 5, 2018

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 5, 2018

📌 Commit 25266c1 has been approved by nikomatsakis

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 7, 2018

Rollup merge of rust-lang#52083 - spastorino:dont-run-ast-borrowck-on…
…-mir-mode, r=nikomatsakis

Dont run ast borrowck on mir mode

r? @nikomatsakis

bors added a commit that referenced this pull request Jul 7, 2018

Auto merge of #52123 - Mark-Simulacrum:rollup, r=Mark-Simulacrum
Rollup of 9 pull requests

Successful merges:

 - #51901 (Rc: remove unused allocation and fix segfault in Weak::new())
 - #52058 (Use of unimplemented!() causing ICE with NLL)
 - #52067 (Visit the mir basic blocks in reverse-postfix order)
 - #52083 (Dont run ast borrowck on mir mode)
 - #52099 (fix typo in stable `--edition` error message)
 - #52103 (Stabilize rc_downcast)
 - #52104 (Remove unnecessary feature gate.)
 - #52117 (Dedupe filetime)
 - #52120 (ARM: expose the "mclass" target feature)

Failed merges:

r? @ghost

@bors bors merged commit 25266c1 into rust-lang:master Jul 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nnethercote

This comment has been minimized.

Copy link
Contributor

nnethercote commented Jul 10, 2018

This was a decent win for performance, speeding up several NLL benchmarks, the best by 9%:
99b0ddb...4f0ca92

We now have apples-to-apples comparisons between the old and new borrow checkers, and the NLL dashboard shows that the new borrow checker is substantially slower. "Check" builds of most benchmarks are at least 10% slower, and two (tuple-stress and html5ever) are still as much as 20x slower; html5ever also has very high memory usage. (#52028)

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.