Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cargo fetch can't be parallelized on nightly anymore #6930

Closed
pietroalbini opened this issue May 10, 2019 · 2 comments
Closed

cargo fetch can't be parallelized on nightly anymore #6930

pietroalbini opened this issue May 10, 2019 · 2 comments
Labels
A-networking Area: networking issues, curl, etc. C-bug Category: bug Command-fetch Performance Gotta go fast!

Comments

@pietroalbini
Copy link
Member

Problem

It used to be possible to run multiple cargo fetch in parallel inside the same cargo home, but it's not possible anymore on the latest nightly: the following fetch invocations just wait for the package cache lock until the other fetches finish.

This is bad for Crater since it slows down fetching deps to 1/8.

Steps

#!/bin/bash
set +euo pipefail

if [[ $# -ne 1 ]]; then
    echo "usage: $0 <toolchain-name>"
    exit 1
fi
export RUSTUP_TOOLCHAIN="$1"

# Use a temporary cargo home so the user-wide one is not messed up
export CARGO_HOME=tmp-cargo-home

set -x

# Create a simple repro crate which depends on lazy_static
cargo init --bin repro
cd repro
echo 'reqwest = "*"' >> Cargo.toml

# Generate the lockfile and fetch the dependencies using it
cargo generate-lockfile
cargo fetch --locked >/dev/null 2>&1 &
sleep 0.1
cargo fetch --locked
  • bash repro.sh stable for the correct behavior
  • bash repro.sh nightly for the wrong behavior

Possible Solution(s)

After talking to @Eh2406 the issue seems to be caused by #6880.

cc @alexcrichton

@pietroalbini pietroalbini added the C-bug Category: bug label May 10, 2019
@alexcrichton
Copy link
Member

This is sort of intentional in the sense that previously Cargo attempted to do fine-grained locking but it became both a performance issue and a correctness burden on Cargo for a use case that very very rarely came up in practice. I believe #6880 is indeed the cause for this though.

@ehuss ehuss added A-networking Area: networking issues, curl, etc. Command-fetch labels May 31, 2019
arnout pushed a commit to buildroot/buildroot that referenced this issue Nov 7, 2022
CARGO_HOME is where Cargo stores its downloaded artefacts. See
https://doc.rust-lang.org/cargo/reference/environment-variables.html:

  CARGO_HOME — Cargo maintains a local cache of the registry index and
  of git checkouts of crates. By default these are stored under
  $HOME/.cargo (%USERPROFILE%\.cargo on Windows), but this variable
  overrides the location of this directory. Once a crate is cached it
  is not removed by the clean command. For more details refer to the
  guide.

We currently make it point to $(HOST_DIR)/share/cargo, but this has a
number of drawbacks:

 (1) It is not shared between Buildroot builds. Each Buildroot build
     will re-download the crates index, and the crates themselves,
     unless of course the final vendored tarball is already there.

 (2) With BR2_PER_PACKAGE_DIRECTORIES=y, it is even worse: CARGO_HOME
     is not even shared between packages, as $(HOST_DIR)/share/cargo
     is per package. So each package in the build that needs vendoring
     of Cargo crates will download the crates index and the crates in
     its own CARGO_HOME location.

To solve this, this commit moves CARGO_HOME into $(DL_DIR), so that it
is shared between builds and packages.

Even though not the best/most authoritative source,
rust-lang/cargo#6930 indicates that there is
a lock when accessing CARGO_HOME, because a user even complains that
this lock has even become more coarse-grained than it used to be
(which for us is fine, it just means that two Cargo fetch operations
from two different packages will be serialized, not a big deal).

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Tested-by: Moritz Bitsch <moritz@h6t.eu>
[yann.morin.1998@free.fr: rename directory: s/\.cargo/br-cargo-home/]
Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
arnout pushed a commit to buildroot/buildroot that referenced this issue Jan 14, 2023
Commit 8450b76 (package/pkg-cargo: move CARGO_HOME into DL_DIR)
allowed for a shared cargo cache of crates. Internally, cargo is
supposed to lock themselves when accessing that cache, and that commit
even had some research in that area, pointing at [0] for complaints
about too-coarse the lock, so it was deemed safe to have a shared cargo
home.

However, in practice, the locking as implemented by cargo, fails to
properly protect the concurrent accesses to the crates cache, with random
failures that manifest themselves like so:

        Blocking waiting for file lock on package cache
        Blocking waiting for file lock on package cache
     Downloading crates ...
    error: failed to sync
    Caused by:
      failed to download packages
    Caused by:
      failed to download `autocfg v1.1.0`
    Caused by:
      unable to get packages from source
    Caused by:
      failed to unpack package `autocfg v1.1.0`
    Caused by:
      failed to unpack entry at `autocfg-1.1.0/src/tests.rs`
    Caused by:
      No such file or directory (os error 2) while canonicalizing [...]

with the last few errors sometime being:

    Caused by:
      failed to parse manifest at `[...]/aho-corasick-0.7.18/Cargo.toml`
    Caused by:
      can't find library `aho_corasick`, rename file to `src/lib.rs` or specify lib.path

So, as we do not systematically use our own cargo build (we can use a
pre-built one with host-rust-bin), we can't patch cargo (even if we knew
what to do!).

Instead, we implement a lock ourselves, by wrapping the call to "cargo
vendor" with a flock(1) on cargo home.

Note: the download wrapper is already flock-ed, but it is a per-package
lock, so it does not prevent different packages from being downloaded in
parallel; if those packages need cargo vendoring, that will not be
protected by the flock on the dl wrapper. So we really do need a flock
on cargo home.

[0] rust-lang/cargo#6930

Fixes: 8450b76

Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Moritz Bitsch <moritz@h6t.eu>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
@epage epage added the Performance Gotta go fast! label Nov 1, 2023
@epage
Copy link
Contributor

epage commented Nov 1, 2023

Considering the trade off involved here and the small scope of what was slowed down (while instead other cases were sped up), I'm not seeing us reverting to the old locking scheme. I'm also not seeing a viable alternative. With that in mind, I'm going to go ahead and close this.

@epage epage closed this as not planned Won't fix, can't repro, duplicate, stale Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-networking Area: networking issues, curl, etc. C-bug Category: bug Command-fetch Performance Gotta go fast!
Projects
None yet
Development

No branches or pull requests

4 participants