1380321 - gkrust and gkrust-gtest are not built in parallel

Status: NEW

(Firefox Build System :: General, enhancement)

Description

[Mike Hommey [:glandium]]

One thing that appears to make builds even longer than they should be on automation is that the gkrust and gkrust-gtest crates are not built in parallel.

The reason this happens is because they are built from 2 different invocations of cargo with the same target directory, and results in a case where cargo just decides to... wait.

    Blocking waiting for file lock on build directory

is what appears in the log when that happens.

The reason both crates use the same target directory is that they have essentially the same dependencies, and having them use different target directories would result in even more time spent compiling even more rust, since everything gkrust would be built twice.

I don't know if there's something we can do on our end without changes to cargo or if cargo has the right set of features to allow us to build both crates in parallel, but considering both are now taking 260s to build each, having them serialized is a huge pain point.

Comment 1

[Alex Crichton [:acrichto]]

Ah yeah concurrent invocations of Cargo basically "don't work" in the sense that they're all serialized quite a bit but at least don't result in broken builds. The fix here is to ensure that `gkrust` and `gkrust-gtest` are in the same workspace and then you can build them in parallel with:

    cargo build -p gkrust -p gkrust-gtest

Does that work in this situation?

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

I wonder whether Cargo can put the build lock on a finer granularity.

I have a separate use case that I usually want to run "mach build -d" and "mach test-unit -p style" at the same time. Although they both build style crate, they are actually different, and should be able to build independently. In this case, cargo is invoked via two different commands, so appending multiple crates on one command isn't going to work.

For Gecko's case (this bug), we also have gkrust and gkrust-gtest in different directory, so they are invoked via different commands as well, but I guess it might be possible that we can hack the build system somehow to make it doing that.

It would still be better if the lock can be finer-grained, I think.

Comment 3

[Mike Hommey [:glandium]]

Indeed, it seems like cargo could be locking the crates that are currently be built. Overall, two cargo could be cooperating and building the crates the other needs.

Comment 4

[Alex Crichton [:acrichto]]

I've opened https://github.com/rust-lang/cargo/issues/4282 to track this in Cargo.

Comment 5

[(not currently active) Ted Mielczarek]

(In reply to Alex Crichton [:acrichto] from comment #1)
> Ah yeah concurrent invocations of Cargo basically "don't work" in the sense
> that they're all serialized quite a bit but at least don't result in broken
> builds. The fix here is to ensure that `gkrust` and `gkrust-gtest` are in
> the same workspace and then you can build them in parallel with:
> 
>     cargo build -p gkrust -p gkrust-gtest
> 
> Does that work in this situation?

We're still not using a cargo workspace for all our crates (I thought I had filed a new bug about that but apparently not?). Even if we did, it sounds like we'd need to change our current cargo invocation strategy to make this work. We currently invoke cargo once per leaf crate. I guess if we did get everything in a single workspace, we could run `cargo build [-p $c]`, listing every leaf crate on the commandline?

Comment 6

[(not currently active) Ted Mielczarek]

(I filed bug 1381576 on trying cargo workspaces again.)

Comment 7

[Alex Crichton [:acrichto]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)
> I guess if we did get
> everything in a single workspace, we could run `cargo build [-p $c]`,
> listing every leaf crate on the commandline?

Indeed that should work!

Comment 8

[Nathan Froyd [:froydnj]]

I think something we could do our end would be to declare that gkrust-gtest depends on gkrust in config/recurse.mk.  That should prevent make from starting two cargo processes at the same time.  Sound plausible?

(I thought I remembered doing this for bug 1349694 locally, but I abandoned it for the approach we have today.  I'm not entirely sure why...)

Comment 9

[Mike Hommey [:glandium]]

The downside of config/recurse.mk is that other backends don't benefit from the knowledge. But yeah, that would work.