Experiments with distributed compilation #249

aidanhs · 2018-05-17T19:35:40Z

An initial version that works and can actually be used to at least try it out.

aidanhs · 2018-08-01T08:47:21Z

Cargo.toml

@@ -45,7 +46,10 @@ rust-crypto = { version = "0.2.36", optional = true }
 serde = "1.0"
 serde_derive = "1.0"
 serde_json = "1.0"
+# TODO: publish to crates.io
+sccache-dist = { path = "sccache-dist" }


I don't know how you'd prefer to deal with this - I can publish and update this PR? Or you can do it next time you release.

Is there a specific reason you're adding this code as a separate crate? It's not really usable standalone, right?

The intention is to split things out into two separate binaries - the sccache-dist binary has a number of things (e.g. rouille) that sccache itself doesn't need to have compiled in.

It also makes the compile a bit more incremental. Are you thinking I could just have it be a separate binary within the same crate?

Yeah, a separate binary within the same crate is what I had envisioned. I get the compile argument (although incremental compilation should help with that nowadays), but managing crates.io releases for two intertwined crates like that is a PITA. (I've dealt with this with another project of mine.)

aidanhs · 2018-08-01T08:49:21Z

src/compiler/gcc.rs

-    // With -fprofile-generate line number information is important, so don't use -P.
-    if !parsed_args.profile_generate {
-        cmd.arg("-P");
-    }


With distributed compilation, line number information is always important. More generally, it's important if you want to use a preprocessed file as input.

Hm. This will regress the change in #208. Would it be possible to change things so that when distributed compilation is not enabled we can continue to use this?

Oh, this is disappointing, I should've done a bit more digging.

I'm not sure if it's better to say "distributed compilation may happen, your cache is completely invalid (because it's based on non-line-number-annotated source files)", or just preprocess a second time on-demand when sending for remote compilation. Looking at a reasonably large file of C++ code I have easily to hand, it's spending 10-20% of wall-clock time in preprocessing so running twice isn't ideal.

What about: if we might be distributing, do the full preprocess with line numbers but strip out the line numbers with a regexp when considering the cache key.

That sounds fine. Since users have to explicitly opt-in to using distributed compilation, I don't think it's unreasonable for them to get this behavior in that case. Running the preprocessor twice seems like too much overhead just for this purpose. We can make a note somewhere if necessary that cache hits using different source paths are not possible with distributed compilation enabled.

aidanhs · 2018-08-01T08:58:05Z

src/compiler/compiler.rs

@@ -286,20 +309,110 @@ pub trait CompilerHasher<T>: fmt::Debug + Send + 'static
    fn box_clone(&self) -> Box<CompilerHasher<T>>;
 }

+#[cfg(not(feature = "dist"))]


This feature is disabled by default, therefore so is distributed compilation

aidanhs · 2018-08-01T08:58:17Z

@luser this is ready for review - it adds the basic structure of two binaries for distributed compilation, the way they communicate and the initial support for C, C++ and Linux. Although this unconditionally shuffles some things around (e.g. the generation of compilation command rather than executing them), distributed compilation is hidden behind a feature flag.

Followups coming soon.

luser · 2018-08-01T19:54:34Z

sccache-dist/src/cache.rs

+
+use errors::*;
+
+// TODO: possibly shouldn't be public


I'm fine with TODO comments, but I'd prefer if they either get removed or if you file follow-up issues and put the issue URL in the comment.

I think all of the TODOs will be gone by the final PR.

luser · 2018-08-01T20:03:50Z

sccache-dist/src/cache.rs

+    Ok(hex(m.finish().as_ref()))
+}
+
+fn hex(bytes: &[u8]) -> String {


This feels like something we could pull in from a crate, like the hex crate or similar.

I just stole it from src/util.rs, I can pull in the crate if you like.

Oh, hah! If you move this code into the existing crate then you can just reuse that code, I guess. I'm certainly not going to hold you to a higher standard than the existing code. :)

luser · 2018-08-01T20:05:11Z

sccache-dist/src/cache.rs

+            .truncate(true)
+            .open("/tmp/toolchain_cache.tar")?;
+        create(file)?;
+        // TODO: after, if still exists, remove it


This sounds like something that should definitely be fixed.

Refined caching and scheduling are a slightly later PR.

luser · 2018-08-01T20:06:09Z

sccache-dist/src/cache.rs

+            return Ok(strong_key)
+        }
+        debug!("Weak key {} appears to be new", weak_key);
+        // TODO: don't use this as a poor exclusive lock on this global file


Does this need to be an exclusive lock? You've already got a mutex around the LRUDiskCache here. If not, using a tempfile would be much nicer.

Added a tempfile (for now)

luser · 2018-08-01T20:12:45Z

sccache-dist/src/cache.rs

+pub struct ClientToolchainCache {
+    cache_dir: PathBuf,
+    cache: Mutex<TcCache>,
+    // Local machine mapping from 'weak' hashes to strong toolchain hashes


Some exposition on what weak and strong hashes are would be helpful. I gather that the strong hash is a SHA512 of the toolchain tar file.

Added the explanation that 'weak' is just what sccache uses as part of the cache key.

aidanhs · 2018-08-04T15:53:34Z

src/server.rs

+        Some(_) => {
+            warn!("Scheduler address configured but dist feature disabled, disabling distributed sccache");
+            Arc::new(dist::NoopClient)
+        },


This bit has added in light of the new preprocessing rules, to use -P in more cases when it's possible.

aidanhs · 2018-08-04T15:54:44Z

src/compiler/gcc.rs

    // With -fprofile-generate line number information is important, so don't use -P.
-    if !parsed_args.profile_generate {
+    if !may_dist && !parsed_args.profile_generate {
        cmd.arg("-P");
    }


Added this back in and explained why it's important for a distributed compile

aidanhs · 2018-08-04T18:30:11Z

sccache-dist has been moved back in-crate as a separate binary, enabled by the dist-server feature.

Are you ok to wait for the rest of my PRs for the TODOs to be resolved?

luser · 2018-08-06T10:40:52Z

Are you ok to wait for the rest of my PRs for the TODOs to be resolved?

Yes. The separate crate part was the only thing I was really hung up on. I'll make sure I've given the rest of the code here a once-over today and we should be good to merge this.

luser · 2018-08-06T19:46:21Z

Cargo.toml

+[[bin]]
+name = "sccache-dist"
+required-features = ["dist-server"]
+path = "sccache-dist/main.rs"


It's a bit unconventional to have source in a path that's not under src/, isn't it?

luser · 2018-08-06T19:49:29Z

lru-disk-cache/src/lib.rs

@@ -202,6 +202,23 @@ impl LruDiskCache {
            })
    }

+    /// Add a file by calling `with` with the open `File` corresponding to the cache at path `key`.
+    pub fn insert_with<K: AsRef<OsStr>, F: FnOnce(File) -> io::Result<()>>(&mut self, key: K, with: F) -> Result<()> {


I'm not quite sure on the precise differences of this vs. insert_by. Can this just call insert_by, passing a closure that opens the file and calls with(f)?

aidanhs · 2018-08-08T18:40:01Z

Added two new commits, one to put sccache-dist in a conventional location (and remove explicit paths to the main.rs files), one to simplify insert_with (this had some minor knock-on effects in insert_by as we don't have the size immediately available).

I also took the opportunity to clean up a couple of things in add_file, as well as add a rel_to_abs_path helper that means we no longer need to store the absolute path in the LRU (since it can be derived from the key).

aidanhs force-pushed the aphs-dist-sccache-1 branch from 8b0bb57 to 963947c Compare June 7, 2018 21:36

aidanhs force-pushed the aphs-dist-sccache-1 branch 2 times, most recently from 22355ca to ef230a7 Compare July 30, 2018 22:27

aidanhs added 27 commits July 30, 2018 23:28

Initial skeletal cleaned up test code

121ee7a

Prototype fuzzing of sccache

818e729

Initial travis attempt

d8d94d5

Misc simplifcations

f222975

Bits and pieces for distributed Rust

9189fb5

Send build outputs back over the wire

2adcb79

Copy the toolchain into the actual location it expects to be in

206c328

C compilers, distribute build inputs by tar

7944f8d

Dispatch actual compiler command

de5b1b8

Distribute and cache toolchains

3bbc803

Fix older rust versions

0c53ad9

Split up build function

fa5f9ed

Fix some warnings

9df03a9

More minimal image for C compilations

dc0e910

Parallelise compilation requests

7c0c7c8

Try harder to parallelise

72606e4

Log tweaks, centralise harcoding

5821e1d

Convert remaining pieces tokio style requests

2bdc88e

Clean up containers on startup

1ffacc5

Bugfixes

7a2bf7b

Some rearrangement and more fixes

dac8881

Disable rust distributed compilation for now

dbee805

Line numbers from preprocessing are necessary for error suppression

62e55cf

Fix three race conditions, all due to racy file access

09f7ff0

More adaptive parallelism

8f33534

Correct environment variable

a3d3458

Convert to using file config

e7a0b73

aidanhs force-pushed the aphs-dist-sccache-1 branch from 605f57f to 997c6e8 Compare August 1, 2018 08:38

Temporarily revert travis.yml changes

9e24d3a

aidanhs commented Aug 1, 2018

View reviewed changes

luser reviewed Aug 1, 2018

View reviewed changes

Review comment forward-ports

191a45b

aidanhs force-pushed the aphs-dist-sccache-1 branch from 81785e5 to 191a45b Compare August 3, 2018 08:48

Move all into single crate

f8af43a

aidanhs force-pushed the aphs-dist-sccache-1 branch from 7f36f9c to 0f799cb Compare August 4, 2018 15:51

aidanhs commented Aug 4, 2018

View reviewed changes

Enable -P if the compile will definitely be local

8fb37aa

aidanhs force-pushed the aphs-dist-sccache-1 branch from 0f799cb to 8fb37aa Compare August 4, 2018 15:54

aidanhs commented Aug 4, 2018

View reviewed changes

aidanhs mentioned this pull request Aug 6, 2018

Support distributed cross-compilation on Windows #271

Merged

luser reviewed Aug 6, 2018

View reviewed changes

aidanhs added 2 commits August 8, 2018 17:59

Move sccache-dist to more conventional location

0982f27

Tweak insert_with, simplify lru values

a80eb5e

luser merged commit 5116315 into mozilla:master Aug 14, 2018

mstange mentioned this pull request Dec 3, 2020

Incorrect line numbers in error messages when using distributed compilation from Windows #893

Closed

earthelf mentioned this pull request Aug 13, 2021

sccache preprocessing with "-P" make debugging into a mess #1026

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experiments with distributed compilation #249

Experiments with distributed compilation #249

aidanhs commented May 17, 2018

aidanhs Aug 1, 2018

luser Aug 1, 2018

aidanhs Aug 2, 2018

luser Aug 3, 2018

aidanhs Aug 1, 2018

luser Aug 1, 2018

aidanhs Aug 2, 2018 •

edited

Loading

luser Aug 3, 2018

aidanhs Aug 1, 2018

aidanhs commented Aug 1, 2018

luser Aug 1, 2018

aidanhs Aug 2, 2018

luser Aug 1, 2018

aidanhs Aug 2, 2018 •

edited

Loading

luser Aug 3, 2018

luser Aug 1, 2018

aidanhs Aug 2, 2018

luser Aug 1, 2018

aidanhs Aug 2, 2018

luser Aug 1, 2018

aidanhs Aug 2, 2018

aidanhs Aug 4, 2018

aidanhs Aug 4, 2018

aidanhs commented Aug 4, 2018

luser commented Aug 6, 2018

luser Aug 6, 2018

luser Aug 6, 2018

aidanhs commented Aug 8, 2018

Experiments with distributed compilation #249

Experiments with distributed compilation #249

Conversation

aidanhs commented May 17, 2018

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

aidanhs Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aidanhs commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aidanhs Aug 2, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aidanhs commented Aug 4, 2018

luser commented Aug 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aidanhs commented Aug 8, 2018

aidanhs Aug 2, 2018 •

edited

Loading

aidanhs Aug 2, 2018 •

edited

Loading