Loading the dependency graph in the background #46560

Yoric · Dec 7, 2017

Patch is a bit longer than I expected, due to the fact that most of this code relies upon a Session value, which is not Sync.

rust-highfive · Dec 7, 2017

r? @michaelwoerister

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

kennytm · Dec 7, 2017

kennytm reviewed Dec 7, 2017

View changes

kennytm · Dec 7, 2017

src/librustc_incremental/persist/load.rs

+
+        if self.cleanup_needed {
+            if let Err(err) = delete_all_session_dir_contents(sess) {
+                sess.err(&format!("Failed to delete invalidated or incompatible incremental compilation session \


Tidy error.

[00:04:24] tidy error: /checkout/src/librustc_incremental/persist/load.rs:95: line longer than 100 chars

kennytm · Dec 7, 2017

src/librustc_incremental/persist/file_format.rs

@@ -60,7 +59,7 @@ pub fn write_file_header<W: io::Write>(stream: &mut W) -> io::Result<()> {
 ///   incompatible version of the compiler.
 /// - Returns `Err(..)` if some kind of IO error occurred while reading the
 ///   file.
-pub fn read_file(sess: &Session, path: &Path) -> io::Result<Option<(Vec<u8>, usize)>> {
+pub fn read_file(report_incremental_info: bool, path: &Path) -> io::Result<Option<(Vec<u8>, usize)>> {


Tidy error.

[00:04:24] tidy error: /checkout/src/librustc_incremental/persist/file_format.rs:62: line longer than 100 chars

michaelwoerister · Dec 7, 2017

michaelwoerister reviewed Dec 7, 2017

View changes

Thank you for your PR, @Yoric!

I've left some comments below. I think the story around LoadResult is not factored quite cleanly yet.

michaelwoerister · Dec 7, 2017

src/librustc_driver/driver.rs

-        let prev_dep_graph = time(time_passes, "load prev dep-graph", || {
+    // If necessary, compute the dependency graph (in the background).
+    let future_dep_graph = if sess.opts.build_dep_graph() {
+        Some(time(time_passes, "load prev dep-graph", || {


This time() call will only measure spawning the thread. I think it would make more sense to measure the duration spent in the new thread.

michaelwoerister · Dec 7, 2017

src/librustc_incremental/persist/load.rs

+pub struct LoadResult<T> {
+    data: T,
+    cleanup_needed: bool,
+    warnings: Option<String>


Since we are calling Session::err() with these, errors would be a better name (warning implies something non-fatal while error will abort the compilation process.

michaelwoerister · Dec 7, 2017

src/librustc_incremental/persist/load.rs

+    match file_format::read_file(report_incremental_info, path) {
+        Ok(Some(data_and_pos)) => LoadResult {
+            data: Some(data_and_pos),
+            ..LoadResult::default()


I usually prefer listing fields exhaustively in situations like this, as it makes the code easier to read.

michaelwoerister · Dec 7, 2017

src/librustc_incremental/persist/load.rs

        Ok(None) => {
            // The file either didn't exist or was produced by an incompatible
            // compiler version. Neither is an error.
+            LoadResult::default()


cleanup_needed must be set to true here (only the Ok(Some(..)) branch returned without the cleanup).

michaelwoerister · Dec 7, 2017

src/librustc_incremental/persist/load.rs

-fn load_data(sess: &Session, path: &Path) -> Option<(Vec<u8>, usize)> {
-    match file_format::read_file(sess, path) {
-        Ok(Some(data_and_pos)) => return Some(data_and_pos),
+pub struct LoadResult<T> {


I feel like this should be an enum. Something like:

enum LoadResult { Ok { data: T }, DataOutOfDate, Error { message: String }, }

michaelwoerister · Dec 7, 2017

src/librustc_incremental/persist/load.rs

-    if sess.opts.incremental.is_none() {
-        return empty
-    }
+    let aux = move || {


Could you make this just one closure that is directly passed to thread::spawn()? Having one closure nested inside of another feels a bit roundabout.

Yoric · Dec 12, 2017

(updated)

Yoric · Dec 14, 2017

I don't really understand the tests that fail :/

michaelwoerister · Dec 14, 2017

These tests make sure that some expected paths in the dep-graph exist and that others don't. That all the output is missing makes me suspect that the graph is not loaded at all or not generated.

michaelwoerister · Dec 14, 2017

michaelwoerister reviewed Dec 14, 2017

View changes

Nice! This looks much cleaner now.

I think the failing tests are because the compiler crashes because we enable the dependency graph in these cases but don't specify a cache directory. This case was previously covered by 2130e5e#diff-8f003505e6f30ad2197a2883c43cb5c2L109.

To verify this, I'll run the tests locally and look at the actual test output (which can be found in text files within the build directory).

michaelwoerister · Dec 14, 2017

src/librustc_incremental/persist/load.rs

+    pub fn open(self, sess: &Session) -> PreviousDepGraph {
+        match self {
+            LoadResult::Error { message } => {
+                sess.err(&message) /* never returns */;


sess.err() actually does return (the compilation process is aborted at a later time, possible after more errors have been reported). But you can just use sess.fatal() to get the desired effect.

michaelwoerister · Dec 14, 2017

src/librustc_incremental/persist/load.rs

+            },
+            LoadResult::DataOutOfDate => {
+                if let Err(err) = delete_all_session_dir_contents(sess) {
+                    sess.err(&format!("Failed to delete invalidated or incompatible \


You can also use sess.fatal() here too.

michaelwoerister · Dec 14, 2017

src/librustc_incremental/persist/load.rs

+{
+    let is_incremental = sess.opts.incremental.is_some();
+    let report_incremental_info = sess.opts.debugging_opts.incremental_info;
+    let path = dep_graph_path_from(&sess.incr_comp_session_dir());


This line might be crashing if the dependency graph is enabled but we don't have a caching directory. This can happen in the failing test cases. I'll verify this later.

michaelwoerister · Dec 14, 2017

It looks like 2130e5e#diff-8f003505e6f30ad2197a2883c43cb5c2R140 really is the problem. Session::incr_comp_session_dir() must not be called if is_incremental is false. The API around this can probably be made nicer, but that's for another PR.

Yoric · Dec 15, 2017

Ok, tests now pass :)

michaelwoerister · Dec 15, 2017

Thanks a lot, @Yoric!

@bors r+

bors · Dec 15, 2017

📌 Commit a0fb93d has been approved by michaelwoerister

bors · Dec 15, 2017

⌛️ Testing commit a0fb93d with merge cfd66a7...

arielb1 · Dec 15, 2017

@bors retry

Prioritizing #45525

bors · Dec 16, 2017

⌛️ Testing commit a0fb93d with merge b3392f8...

bors · Dec 16, 2017

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

Issue #46555 - Making IndexVec Send if its contents are Send

8500fb9

rust-highfive assigned michaelwoerister Dec 7, 2017

kennytm added the S-waiting-on-review label Dec 7, 2017

carols10cents added S-waiting-on-author and removed S-waiting-on-review labels Dec 12, 2017

Yoric force-pushed the Yoric:incr branch 2 times, most recently from e3a6b96 to 2130e5e Dec 12, 2017

kennytm added S-waiting-on-review and removed S-waiting-on-author labels Dec 14, 2017

Yoric force-pushed the Yoric:incr branch 2 times, most recently from 7d3c8f2 to 67a085c Dec 15, 2017

Resolves #46555 - Moving loading and decoding of dependency graph to …
…background thread

Loading status checks…

a0fb93d

Yoric force-pushed the Yoric:incr branch from 67a085c to a0fb93d Dec 15, 2017

bors merged commit a0fb93d into rust-lang:master Dec 16, 2017
2 checks passed

2 checks passed

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

homu Test successful
Details

rust-lang/rust

Join GitHub today

Loading the dependency graph in the background #46560

Conversation

Yoric commented Dec 7, 2017

rust-highfive assigned michaelwoerister Dec 7, 2017

This comment has been minimized.

rust-highfive commented Dec 7, 2017

kennytm added the S-waiting-on-review label Dec 7, 2017

kennytm reviewed Dec 7, 2017 View changes

This comment has been minimized.

This comment has been minimized.

michaelwoerister reviewed Dec 7, 2017 View changes

michaelwoerister left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

carols10cents added S-waiting-on-author and removed S-waiting-on-review labels Dec 12, 2017

This comment has been minimized.

Yoric commented Dec 12, 2017

Yoric force-pushed the Yoric:incr branch 2 times, most recently from e3a6b96 to 2130e5e Dec 12, 2017

This comment has been minimized.

Yoric commented Dec 14, 2017

kennytm added S-waiting-on-review and removed S-waiting-on-author labels Dec 14, 2017

This comment has been minimized.

michaelwoerister commented Dec 14, 2017

michaelwoerister reviewed Dec 14, 2017 View changes

michaelwoerister left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

michaelwoerister commented Dec 14, 2017

Yoric force-pushed the Yoric:incr branch 2 times, most recently from 7d3c8f2 to 67a085c Dec 15, 2017

Yoric force-pushed the Yoric:incr branch from 67a085c to a0fb93d Dec 15, 2017

This comment has been minimized.

Yoric commented Dec 15, 2017

This comment has been minimized.

michaelwoerister commented Dec 15, 2017

This comment has been minimized.

bors commented Dec 15, 2017

This comment has been minimized.

bors commented Dec 15, 2017

bors added a commit that referenced this pull request Dec 15, 2017

This comment has been minimized.

arielb1 commented Dec 15, 2017

This comment has been minimized.

bors commented Dec 16, 2017

bors added a commit that referenced this pull request Dec 16, 2017

This comment has been minimized.

bors commented Dec 16, 2017

Hide details View details bors merged commit a0fb93d into rust-lang:master Dec 16, 2017 2 checks passed

2 checks passed

kennytm reviewed Dec 7, 2017

View changes

michaelwoerister reviewed Dec 7, 2017

View changes

Yoric force-pushed the Yoric:incr branch 2 times, most recently from `e3a6b96` to `2130e5e` Dec 12, 2017

michaelwoerister reviewed Dec 14, 2017

View changes

Yoric force-pushed the Yoric:incr branch 2 times, most recently from `7d3c8f2` to `67a085c` Dec 15, 2017

Yoric force-pushed the Yoric:incr branch from `67a085c` to `a0fb93d` Dec 15, 2017

bors merged commit `a0fb93d` into rust-lang:master Dec 16, 2017
2 checks passed