Integrate jobserver support to parallel codegen #42682

alexcrichton · Jun 15, 2017

This commit integrates the jobserver crate into the compiler. The crate was
previously integrated in to Cargo as part of rust-lang/cargo#4110. The purpose
here is to two-fold:

Primarily the compiler can cooperate with Cargo on parallelism. When you run
cargo build -j4 then this'll make sure that the entire build process between
Cargo/rustc won't use more than 4 cores, whereas today you'd get 4 rustc
instances which may all try to spawn lots of threads.
Secondarily rustc/Cargo can now integrate with a foreign GNU make jobserver.
This means that if you call cargo/rustc from make or another
jobserver-compatible implementation it'll use foreign parallelism settings
instead of creating new ones locally.

As the number of parallel codegen instances in the compiler continues to grow
over time with the advent of incremental compilation it's expected that this'll
become more of a problem, so this is intended to nip concurrent concerns in the
bud by having all the tools to cooperate!

Note that while rustc has support for itself creating a jobserver it's far more
likely that rustc will always use the jobserver configured by Cargo. Cargo today
will now set a jobserver unconditionally for rustc to use.

rust-highfive · Jun 15, 2017

r? @arielb1

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

alexcrichton · Jun 15, 2017

r? @michaelwoerister

michaelwoerister · Jun 16, 2017

michaelwoerister reviewed Jun 16, 2017

View changes

Very nice! I'm excited about this :)

Regarding the implementation, I'm not quite clear on how token handling works there. Wouldn't it be easier to just move one token into each spawn_work and let it go out of scope there?

michaelwoerister · Jun 16, 2017

src/librustc_trans/back/lto.rs

@@ -82,16 +84,11 @@ pub fn run(sess: &session::Session,
    // For each of our upstream dependencies, find the corresponding rlib and
    // load the bitcode from the archive. Then merge it into the current LLVM
    // module that we've got.
-    link::each_linked_rlib(sess, &mut |cnum, path| {
-        // `#![no_builtins]` crates don't participate in LTO.
-        if sess.cstore.is_no_builtins(cnum) {


Did you remove this on purpose?

I see, it's added in later again.

Yeah this query just ended up having a lot of dependencies on sess so I figured it'd be best to move it way up to the beginning instead of only running it back here.

michaelwoerister · Jun 16, 2017

src/librustc_trans/back/write.rs

-                        execute_work_item(&cgcx, work);
+    let mut tokens = Vec::new();
+    let mut running = 0;
+    while work_items.len() > 0 || running > 0 {


Could you add a comment here saying something to the effect of "This is our 'main loop', taking care of spawning worker threads and communicating with live ones via message passing -- so we have to keep it running as long as there's still work that hasn't been doled out to a worker (work_items > 0) or if there are still live workers to be communicated with (running > 0)."

michaelwoerister · Jun 16, 2017

src/librustc_trans/back/write.rs

+                       scope,
+                       tx.clone(),
+                       work_items.pop().unwrap(),
+                       work_items.len());


I'm not very fond this: mutating work_items via pop and then taking its len. I assume that we have a defined evaluation order of function arguments, but I don't like relying on it.

michaelwoerister · Jun 16, 2017

src/librustc_trans/back/write.rs

+        // possible. Remember that we have an ambient token available to us
+        // hence the `+1` here.
+        //
+        // Also note that we may actually acquire more tokens than we need, so


When does that happen? If we abort early because of an error?

michaelwoerister · Jun 16, 2017

src/librustc_trans/back/write.rs

+        //
+        // Also note that we may actually acquire more tokens than we need, so
+        // in that case just truncate the `tokens` list every time we pass
+        // through here.


Could you add that truncating implies dropping and thus releasing tokens?

michaelwoerister · Jun 16, 2017

src/librustc_trans/back/write.rs

+                       work_items.len());
+            running += 1;
+        }
+        tokens.truncate(running.saturating_sub(1));


I'm not quite sure how this works. Can't this cause tokens to be lost without a spawn_work having been called for them?

michaelwoerister · Jun 16, 2017

src/librustc_trans/back/write.rs

+
+        // Set up a destructor which will fire off a message that we're done as
+        // we exit.
+        struct Bomb {


We should have something like this in libstd.

michaelwoerister · Jun 16, 2017

src/librustc_trans/back/write.rs

+        if sess.cstore.is_no_builtins(cnum) {
+            return
+        }
+        each_linked_rlib.push((cnum, path.to_path_buf()));


If the each_linked_rlib field is LTO-specific, we should probably change to the name to reflect this.

michaelwoerister · Jun 16, 2017

src/librustc_trans/back/write.rs

+        // Execute the work itself, and if it finishes successfully then flag
+        // ourselves as a success as well.
+        if execute_work_item(&cgcx, work).is_err() {
+            drop(cgcx.tx.send(Message::AbortIfErrors));


One could argue that it would be cleaner to also mem::forget the bomb in this case.

Yeah I wasn't quite sure how this should be handled, I think that if you see a FatalError then a diagnostic has already been sent off, which in turn already sent AbortIfErrors. In that sense it may be fruitless to send another message here, so I'll just ignore the result.

michaelwoerister · Jun 16, 2017

src/librustc_trans/scope.rs

+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+//! Scoped threads, copied from `crossbeam`


Could we also use crossbeam directly?

Hm upon further inspection, I don't see why not!

alexcrichton · Jun 16, 2017

Ok, updated! @michaelwoerister I added a large comment above the "main loop" which I believe should answer your questions about the token management, but if you'd like me to clarify anything please just let me know!

bors · Jun 18, 2017

☔️ The latest upstream changes (presumably #42676) made this pull request unmergeable. Please resolve the merge conflicts.

michaelwoerister · Jun 19, 2017

michaelwoerister reviewed Jun 19, 2017

View changes

michaelwoerister · Jun 19, 2017

src/librustc_trans/back/write.rs

+    // manner we can ensure that the maximum number of parallel workers is
+    // capped at any one point in time.
+    //
+    // The jobserver protocol is a little unique, however. We, as a running


Because concurrent programming isn't complicated enough by itself already 😛

michaelwoerister · Jun 19, 2017

Thanks for the clarifying comment about the jobserver protocol!

r=me once the merge conflict is fixed.

alexcrichton · Jun 19, 2017

@bors: r=michaelwoerister

bors · Jun 19, 2017

📌 Commit 5d00e5e has been approved by michaelwoerister

bors · Jun 20, 2017

⌛️ Testing commit 5d00e5e with merge 6cb3b99...

bors · Jun 20, 2017

💔 Test failed - status-appveyor

alexcrichton · Jun 20, 2017

@bors: r=michaelwoerister

bors · Jun 20, 2017

📌 Commit a014634 has been approved by michaelwoerister

bors · Jun 20, 2017

⌛️ Testing commit a014634 with merge 0164969...

bors · Jun 20, 2017

💔 Test failed - status-appveyor

Mark-Simulacrum · Jun 20, 2017

Linking failure?

[01:33:02] error: linking with `link.exe` failed: exit code: 1120
[01:33:02]   |
[01:33:02]   = note: "C:\\Program Files (x86)\\Microsoft Visual Studio 14.0\\VC\\bin\\amd64\\link.exe" "/NOLOGO" "/NXCOMPAT" "/LIBPATH:C:\\projects\\rust\\build\\x86_64-pc-windows-msvc\\stage2\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib" "
C:\\projects\\rust\\build\\x86_64-pc-windows-msvc\\test\\run-pass\\smallest-hello-world.0.o" "/OUT:C:\\projects\\rust\\build\\x86_64-pc-windows-msvc\\test\\run-pass\\smallest-hello-world.stage2-x86_64-pc-windows-msvc.exe" "/OPT:REF,ICF" "
/DEBUG" "/LIBPATH:C:\\projects\\rust\\build\\x86_64-pc-windows-msvc\\test\\run-pass" "/LIBPATH:C:\\projects\\rust\\build\\x86_64-pc-windows-msvc\\test\\run-pass\\smallest-hello-world.stage2-x86_64-pc-windows-msvc.run-pass.libaux" "/LIBPAT
H:C:\\projects\\rust\\build\\x86_64-pc-windows-msvc\\native\\rust-test-helpers" "/LIBPATH:C:\\projects\\rust\\build\\x86_64-pc-windows-msvc\\stage2\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib" "C:\\projects\\rust\\build\\x86_64-pc-windows-
msvc\\stage2\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib\\liballoc_system-ace597b8fd7407ce.rlib" "C:\\projects\\rust\\build\\x86_64-pc-windows-msvc\\stage2\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib\\libcore-216e0f775a886879.rlib"
[01:33:02]   = note: smallest-hello-world.0.o : error LNK2019: unresolved external symbol puts referenced in function main
[01:33:02]           LINK : error LNK2001: unresolved external symbol mainCRTStartup
[01:33:02]           C:\projects\rust\build\x86_64-pc-windows-msvc\test\run-pass\smallest-hello-world.stage2-x86_64-pc-windows-msvc.exe : fatal error LNK1120: 2 unresolved externals
[01:33:02]
[01:33:02]
[01:33:02] error: aborting due to previous error(s)
[01:33:02]

alexcrichton · Jun 20, 2017

@bors: r=michaelwoerister

bors · Jun 20, 2017

📌 Commit 2376e0d has been approved by michaelwoerister

bors · Jun 21, 2017

🔒 Merge conflict

bors · Jun 21, 2017

☔️ The latest upstream changes (presumably #42664) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton · Jun 21, 2017

@bors: r=michaelwoerister

bors · Jun 21, 2017

📌 Commit 451d392 has been approved by michaelwoerister

alexcrichton · Jun 21, 2017

@bors: r=michaelwoerister

bors · Jun 21, 2017

📌 Commit 201f069 has been approved by michaelwoerister

bors · Jun 21, 2017

⌛️ Testing commit 201f069 with merge 694adee...

bors · Jun 21, 2017

💔 Test failed - status-travis

alexcrichton · Jun 21, 2017

@bors: retry * osx timed out

…

On Wed, Jun 21, 2017 at 4:24 PM, bors ***@***.***> wrote: 💔 Test failed - status-travis <https://travis-ci.org/rust-lang/rust/builds/245482773?utm_source=github_status&utm_medium=notification> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#42682 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95FEbgYHMXulmMHTszBVJR6hpkE9Lks5sGYoOgaJpZM4N7l7L> .

bors · Jun 22, 2017

⌛️ Testing commit 201f069 with merge 80271e8...

bors · Jun 22, 2017

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

jdm · Jun 28, 2017

So does this only support invoking cargo/rustc from make, but the behaviour of invoking make from a Cargo build script is unchanged?

alexcrichton · Jun 30, 2017

@jdm it's a little more nuanced than that. Cargo also creates a jobserver in addition to consuming one, meaning that rustc will basically always use that jobserver now. If Cargo inherits a jobserver though then rustc likely will too.

You need to tweak makefiles calling rustc/cargo though to actually let them inherit the jobserver, notably adding a + to the beginning of the rule definition.

For build scripts invoking make the make subprocess will inherit Cargo's jobserver if no -j argument is passed, but if -jN is passed then that'll override the inherited jobserver.

rust-highfive assigned arielb1 Jun 15, 2017

rust-highfive assigned michaelwoerister and unassigned arielb1 Jun 15, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch 3 times, most recently from c765aad to 4e8e13a Jun 15, 2017

shepmaster added the S-waiting-on-author label Jun 16, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch from 4e8e13a to b364714 Jun 16, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch from b364714 to 0f66436 Jun 17, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch from 0f66436 to 5d00e5e Jun 19, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch from 5d00e5e to a014634 Jun 20, 2017

arielb1 added S-waiting-on-bors and removed S-waiting-on-author labels Jun 20, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch from a014634 to 2376e0d Jun 20, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch from 2376e0d to 451d392 Jun 21, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch from 451d392 to 46dc6da Jun 21, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch from 46dc6da to 201f069 Jun 21, 2017

bors merged commit 201f069 into rust-lang:master Jun 22, 2017
2 checks passed

2 checks passed

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

homu Test successful
Details

This was referenced Jun 22, 2017

Merged

rustc: Implement stack probes for x86 #42816

Merged

rustc: Implement the #[global_allocator] attribute #42727

alexcrichton deleted the alexcrichton:jobserver branch Jun 22, 2017

rillian referenced this pull request Jun 23, 2017
Closed
ICE compiling rustc-serialize under sccache 'failed to acquire jobserver token' #42867

jdm referenced this pull request Jun 30, 2017
Open
Use cargo's jobserver instead of specifying -j manually #120

rust-lang/rust

Join GitHub today

Integrate jobserver support to parallel codegen #42682

Conversation

alexcrichton commented Jun 15, 2017

rust-highfive assigned arielb1 Jun 15, 2017

This comment has been minimized.

rust-highfive commented Jun 15, 2017

This comment has been minimized.

alexcrichton commented Jun 15, 2017

rust-highfive assigned michaelwoerister and unassigned arielb1 Jun 15, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch 3 times, most recently from c765aad to 4e8e13a Jun 15, 2017

michaelwoerister reviewed Jun 16, 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.

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.

This comment has been minimized.

This comment has been minimized.

shepmaster added the S-waiting-on-author label Jun 16, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch from 4e8e13a to b364714 Jun 16, 2017

This comment has been minimized.

alexcrichton commented Jun 16, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch from b364714 to 0f66436 Jun 17, 2017

This comment has been minimized.

bors commented Jun 18, 2017

michaelwoerister reviewed Jun 19, 2017 View changes

This comment has been minimized.

This comment has been minimized.

michaelwoerister commented Jun 19, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch from 0f66436 to 5d00e5e Jun 19, 2017

This comment has been minimized.

alexcrichton commented Jun 19, 2017

This comment has been minimized.

bors commented Jun 19, 2017

This comment has been minimized.

bors commented Jun 20, 2017

bors added a commit that referenced this pull request Jun 20, 2017

This comment has been minimized.

bors commented Jun 20, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch from 5d00e5e to a014634 Jun 20, 2017

This comment has been minimized.

alexcrichton commented Jun 20, 2017

This comment has been minimized.

bors commented Jun 20, 2017

This comment has been minimized.

bors commented Jun 20, 2017

bors added a commit that referenced this pull request Jun 20, 2017

arielb1 added S-waiting-on-bors and removed S-waiting-on-author labels Jun 20, 2017

This comment has been minimized.

bors commented Jun 20, 2017

This comment has been minimized.

Mark-Simulacrum commented Jun 20, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch from a014634 to 2376e0d Jun 20, 2017

This comment has been minimized.

alexcrichton commented Jun 20, 2017

This comment has been minimized.

bors commented Jun 20, 2017

This comment has been minimized.

bors commented Jun 21, 2017

This comment has been minimized.

bors commented Jun 21, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch from 2376e0d to 451d392 Jun 21, 2017

This comment has been minimized.

alexcrichton commented Jun 21, 2017

This comment has been minimized.

bors commented Jun 21, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch from 451d392 to 46dc6da Jun 21, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch from 46dc6da to 201f069 Jun 21, 2017

This comment has been minimized.

alexcrichton commented Jun 21, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch 3 times, most recently from `c765aad` to `4e8e13a` Jun 15, 2017

michaelwoerister reviewed Jun 16, 2017

View changes

alexcrichton force-pushed the alexcrichton:jobserver branch from `4e8e13a` to `b364714` Jun 16, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch from `b364714` to `0f66436` Jun 17, 2017

michaelwoerister reviewed Jun 19, 2017

View changes

alexcrichton force-pushed the alexcrichton:jobserver branch from `0f66436` to `5d00e5e` Jun 19, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch from `5d00e5e` to `a014634` Jun 20, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch from `a014634` to `2376e0d` Jun 20, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch from `2376e0d` to `451d392` Jun 21, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch from `451d392` to `46dc6da` Jun 21, 2017

alexcrichton force-pushed the alexcrichton:jobserver branch from `46dc6da` to `201f069` Jun 21, 2017

bors merged commit `201f069` into rust-lang:master Jun 22, 2017
2 checks passed