Loading the dependency graph in the background #46560

Yoric · 2017-12-07T15:17:54Z

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 · 2017-12-07T15:18:06Z

r? @michaelwoerister

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

kennytm · 2017-12-07T15:48:02Z

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 · 2017-12-07T15:48:37Z

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

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 · 2017-12-07T16:26:51Z

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 · 2017-12-07T16:31:00Z

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 · 2017-12-07T16:33:30Z

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 · 2017-12-07T16:35:23Z

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 · 2017-12-07T17:42:14Z

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 · 2017-12-07T17:44:20Z

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 · 2017-12-12T22:08:22Z

(updated)

Yoric · 2017-12-14T19:15:00Z

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

michaelwoerister · 2017-12-14T21:01:14Z

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

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 · 2017-12-14T21:55:37Z

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 · 2017-12-14T21:56:20Z

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 · 2017-12-14T22:01:13Z

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 · 2017-12-14T22:32:07Z

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.

…graph to background thread

Yoric · 2017-12-15T20:18:06Z

Ok, tests now pass :)

michaelwoerister · 2017-12-15T20:59:05Z

Thanks a lot, @Yoric!

@bors r+

bors · 2017-12-15T20:59:06Z

📌 Commit a0fb93d has been approved by michaelwoerister

bors · 2017-12-15T21:48:47Z

⌛ Testing commit a0fb93d with merge cfd66a768a71a322d1d0e0ffc6116db5ad4b3b33...

arielb1 · 2017-12-15T21:56:57Z

@bors retry

Prioritizing #45525

bors · 2017-12-16T03:54:16Z

⌛ Testing commit a0fb93d with merge b3392f8...

Loading the dependency graph in the background 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`.

bors · 2017-12-16T06:31:26Z

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

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

8500fb9

rust-highfive assigned michaelwoerister Dec 7, 2017

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 7, 2017

kennytm reviewed Dec 7, 2017

View reviewed changes

michaelwoerister reviewed Dec 7, 2017

View reviewed changes

carols10cents 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 Dec 12, 2017

Yoric force-pushed the incr branch 2 times, most recently from e3a6b96 to 2130e5e Compare December 13, 2017 19:40

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

michaelwoerister reviewed Dec 14, 2017

View reviewed changes

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

Resolves rust-lang#46555 - Moving loading and decoding of dependency …

a0fb93d

…graph to background thread

Yoric force-pushed the incr branch from 67a085c to a0fb93d Compare December 15, 2017 18:02

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loading the dependency graph in the background #46560

Loading the dependency graph in the background #46560

Yoric commented Dec 7, 2017

rust-highfive commented Dec 7, 2017

kennytm Dec 7, 2017

kennytm Dec 7, 2017

michaelwoerister left a comment

michaelwoerister Dec 7, 2017

michaelwoerister Dec 7, 2017

michaelwoerister Dec 7, 2017

michaelwoerister Dec 7, 2017

michaelwoerister Dec 7, 2017

michaelwoerister Dec 7, 2017

Yoric commented Dec 12, 2017

Yoric commented Dec 14, 2017

michaelwoerister commented Dec 14, 2017

michaelwoerister left a comment

michaelwoerister Dec 14, 2017

michaelwoerister Dec 14, 2017

michaelwoerister Dec 14, 2017

michaelwoerister commented Dec 14, 2017

Yoric commented Dec 15, 2017

michaelwoerister commented Dec 15, 2017

bors commented Dec 15, 2017

bors commented Dec 15, 2017

arielb1 commented Dec 15, 2017

bors commented Dec 16, 2017

bors commented Dec 16, 2017

Loading the dependency graph in the background #46560

Loading the dependency graph in the background #46560

Conversation

Yoric commented Dec 7, 2017

rust-highfive commented Dec 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelwoerister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yoric commented Dec 12, 2017

Yoric commented Dec 14, 2017

michaelwoerister commented Dec 14, 2017

michaelwoerister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelwoerister commented Dec 14, 2017

Yoric commented Dec 15, 2017

michaelwoerister commented Dec 15, 2017

bors commented Dec 15, 2017

bors commented Dec 15, 2017

arielb1 commented Dec 15, 2017

bors commented Dec 16, 2017

bors commented Dec 16, 2017