Performance Regression in `cargo package` on `1.81.0`

[landonxjames]

Problem

The aws-sdk-rust recently updated our MSRV to 1.81.0. This caused a 3-4x slowdown in our cargo package invocation. We traced the likely culprit of this slowdown to https://github.com/rust-lang/cargo/pull/13960 which removes an if !opts.allow_dirty check around the check_repo_state function causing it to run on ever packaging step.

This causes a problem with the aws-sdk-rust repo. Since it is very large and has a huge history all invocations to git in that repository are slow. Since cargo package is now invoking git on every packaging step in the workspace it slows the whole process down to a crawl.

Steps

1. Run git clone https://github.com/awslabs/aws-sdk-rust.git to checkout the aws-sdk-rust repo (this will be a bit slow, it is huge)
2. Ensure that you are using a cargo version <1.81
3. In the aws-sdk-rust repo run cargo package --no-verify --allow-dirty --workspace observe the approximate pace at which crates are packaged (you can also use the time command, but this fails locally for me with Too many open files (os error 24) before it completes)
4. Upgrade your cargo version to =1.81
5. Again run time cargo package --no-verify --allow-dirty --workspace (this goes too slowly for me to wait for it to fail, but it is very easy to observe the difference in speed)

Possible Solution(s)

Potentially when packaging multiple crates it might be possible to only invoke git once at the beginning of the run? Or potentially add a new option to disable the behavior introduced in https://github.com/rust-lang/cargo/pull/13960 that always generates a .cargo_vcs_info.json

Notes

No response

Version

$ cargo version --verbose
cargo 1.81.0 (2dbb1af80 2024-08-20)
release: 1.81.0
commit-hash: 2dbb1af80a2914475ba76827a312e29cedfa6b2f
commit-date: 2024-08-20
host: aarch64-apple-darwin
libgit2: 1.8.1 (sys:0.19.0 vendored)
libcurl: 8.7.1 (sys:0.4.73+curl-8.8.0 system ssl:(SecureTransport) LibreSSL/3.3.6)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: Mac OS 14.7.1 [64-bit]

[epage]

Potentially when packaging multiple crates it might be possible to only invoke git once at the beginning of the run?

The dirty check specifically checks for whether any files being packaged are dirty which makes at least part of this a per-package operation. However, may parts could be skipped or moved earlier, depending on where the slow down is.

[weihanglo]

https://github.com/rust-lang/cargo/pull/13960 is the culprit of this regression. We do git ls-files for every package unconditionally after that PR. When -Zpackage-workspace is stabilized, the issue will be more prominent.

[weihanglo]

Going to do a refactor on moving this out from the package loop.

@rustbot claims

[weihanglo]

Did some profiling. Detailed report in https://github.com/rust-lang/cargo/pull/14962.

To summarize, we were doing git status and comparing dirty files against each package to be published. aws-rust-sdk has 400+ members so such dirty comparison has repeated 400+ times.

Here are profiling data: trace.tar.gz

On 59b2ddd4484 (trace-offline.json)

#14962 (trace-offline-pathspec.json)

[weihanglo]

If we skipped the entire git status check (the --allow-dirty behavior prior to #13960), the profiling data would look like this:

[weihanglo]

#14962 was closed because we found more bugs in cargo package VCS dirtiness check, showing in #14967. If #14962 merged those bugs would be harder to fix.

[epage]

@weihanglo if we assume most repos are clean, could we ask git if the repo is clean and bypass these checks, only doing them if the repo is dirty?

[weihanglo]

if we assume most repos are clean, could we ask git if the repo is clean and bypass these checks, only doing them if the repo is dirty?

Cargo does that today already (+ untracked and ignored).

https://github.com/rust-lang/cargo/blob/f73259dbff1f8db90d9cf6b0fd5e32c2ff116660/src/cargo/ops/cargo_package/vcs.rs#L170-L176

As of the current HEAD f73259dbff1f8db90d9cf6b0fd5e32c2ff116660, PathSource::list_files performs a dirwalk and doesn't really compare status between Git index and worktree. I don't know how to combine one dirwalk + git status --untracked --ignored into one git status call in list_files(), and whether it is worthy.

[epage]

I got mixed up; I thought git2 had an API for "is repo dirty" but what I was thinking of is "repo state" which is unrelated (though feels wrong to release from anything but a Clean state)

[weihanglo]

A complete fix of the regression might be Cargo offering a --no-vcs-info, which skips every checks about VCS. I can't see any other approach could really restore the same performance as before.

[landonxjames]

A complete fix of the regression might be Cargo offering a --no-vcs-info, which skips every checks about VCS. I can't see any other approach could really restore the same performance as before.

We would be fine with this fix, the vcs-info isn't really useful for us, so the ability to remove it (and along with it all git invocations) would suit our purposes.

[epage]

imo providing --no-vcs-info should be a last resort

• This extends the CLI for a fairly limited case where the performance matters and we couldn't help it in other ways. The larger the CLI, the harder it is for people to find things that are relevant
• There is a lot of interest in packages published to crates.io to have VCS info to check to see if anything has changed between VCS and publish. Improving that situation is the reason we have this performance regression (#13960).

imo before we consider alternatives, we should see what the limit is for how much we can optimize what we are already doing and see if that is within an acceptable limit (determined by the Cargo team).

If we do look for more alternatives, some additional ideas include:

• Generate VCS info but skip dirty info with --allow-dirty (this seems a bit magical)
• Remove dirty info from VCS info: its advisory only anyways

[landonxjames]

If anyone else happens to hit this issue, we did find a workaround. In our publishing step we now rm -rf .git/ before running cargo package and that has taken a full workspace publish from ~3 hours to ~15 minutes. Would still be nice to have this fixed in the cargo package command itself, but we are back to around the performance we had previously.

[weihanglo]

In our publishing step we now rm -rf .git/ before running cargo package and that has taken a full workspace publish from ~3 hours to ~15 minutes.

That is indeed a quick workaround. Just mind that Cargo packages stuff based on git index. If .git is gone, it falls back to walking the directory of a package. That could lead to a different packaging behavior if there is something previously ignored or untracked by git.

[epage]

Any word on what the performance is like with #14997 merged? Might be a bit more difficult to test until its in a nightly.

It would be helpful to know what the performance was pre-1.81, post-1.81, and with this change and whether the new performance is at an acceptable level.

[weihanglo]

@epage See https://github.com/rust-lang/cargo/issues/14955#issuecomment-2555139730 for a glance. I believe it would be at least 3x faster by just looking the proportion of each trace.

[Byron]

Good news: #15534 seems to work as a fix for the regression:

With the current version there are results immediately and it takes a little less than 12m to compile everything. The greatest expense seems to be the fetching of latest packages now.

aws-sdk-rust ( main) via 🐍
❯ /Users/byron/dev/github.com/rust-lang/cargo/target/release/cargo package --allow-dirty --no-verify --no-metadata
[..]
   Packaging aws-smithy-observability-otel v0.1.0 (/Users/byron/dev/github.com/awslabs/aws-sdk-rust/sdk/aws-smithy-observability-otel)
    Updating crates.io index
    Packaged 12 files, 83.4KiB (20.6KiB compressed)
   Packaging aws-smithy-types-convert v0.60.9 (/Users/byron/dev/github.com/awslabs/aws-sdk-rust/sdk/aws-smithy-types-convert)
    Updating crates.io index
    Packaged 10 files, 36.9KiB (11.0KiB compressed)
   Packaging aws-smithy-wasm v0.1.4 (/Users/byron/dev/github.com/awslabs/aws-sdk-rust/sdk/aws-smithy-wasm)
    Updating crates.io index
    Packaged 9 files, 40.2KiB (12.7KiB compressed)

aws-sdk-rust ( main) via 🐍 took 11m49s

The changes require the gitoxide version to be used, but if someone is interested, there should be no problem in backporting the changes to the git2 version.

[epage]

From the last update, it sounds like this is now resolved so I'm closing. If there is a reason for us to re-evaluate, let us know!

[weihanglo]

This is still not entirely fixed. The latest update made it less bad, but you still need to collect status for all repos

[Byron]

Just as a note: to my mind, this can be solved by reorganising the loop so it does the full status at the beginning, and then stores status results in something like this (I'd hope I can make this available via gix as well). Then it's trivial to lookup status items from a cache/lut for each sub-package instead of obtaining an entire status. This would then be the fastest option for sure, while scaling to pathological cases.