Use gix for cargo package

[Byron]

This should also help fixing these spurious "cannot package because some excluded file is untracked" issues.

Tasks

• step-by-step conversion of vcs.rs
• use proper feature toggle
• cleanup final check by myself
• move split & rename into its own commit. Probably squash all changes except for the gix upgrade.
  • I like to have the major stages of this PR conserved.
• upgrade to a gix release including various improvements GitoxideLabs/gitoxide#2016
  • This was done in master already.
• fix tests by fixing gix - submodules() call isn't bare-repo safe.
• fix failure on Windows
  • gix status seems to go through a symlink, arriving at the wrong conclusion, on Windows.
• fix performance regression on aws-sdk-rust.

Notes for the Reviewer

• This implementation is both faster and more correct, thus affects fix(package): detect dirtiness for symlinks to submodule  #15416 and Performance Regression in cargo package on 1.81.0 #14955.

Related to GitoxideLabs/gitoxide#106.

[weihanglo]

Thanks for kicking of this!

Regardless the approach taken here. I wonder if it is possible to avoid checking repo state completely. In PathSource::list_files we already walked through it. And maybe we can attach extra info in PathEntry. Of course it'll slow down list_files a bit, but with that we could perhaps resolve #14941?.

It is just a bit relevant to this PR though. We can still merge this when ready.

[Byron]

It's about time I get started with this - this setup here is super-hacky, but it's designed to allow a very step-wise conversion, hopefully allowing to work on it 15 minutes at a time.

Regarding #14941, I hope with #14955 I have found a representative issue. Right now I can't tell how that should be solved, so I'd probably go for having a gitoxide alternative for cargo package here and tackle the related issues another time. Ideally, the new implementation will be a bit faster and I hope I can test that on the aws-sdk-rust repository.

[Byron]

I couldn't resist to profile the aws-sdk-rust publishing step in Cargo and here is a picture of the run so far.

I don't know if this will ever finish, and I don't know how they can even get this published, maybe it just takes a very long time.

The culprit, maybe among other things, may be repo.status_file() calls for which there isn't an equivalent in gitoxide anyway. Thus I think this will look very differently when this PR is done and maybe that's an avenue for aws-sdk-rust to publish much faster.

I will definitely test that once it's working and share results :).

[weihanglo]

Oops I linked to the wrong issue. Was meant to say #14955

[Byron]

I just realized that dirty_files_outside_pkg_root is probably the reason for the individual git_status_file calls.
The gitoxide implementation can certainly be backported to the git2 one if there is demand, so potential gains could be available sooner.

[Byron]

Also I realize that even with these improvements, and without --no-vcs-info or similar it will still do one git-status costing about 1s each for each of these 404 crates, wasting nearly 7 minutes. Nonetheless, it's probably acceptable compared to the over three ours that it took otherwise (which would be equivalent to ~10800 git status checks).

[Byron]

And I think #15416 is also properly fixed now, which also means the output is different in 2 tests when using the gitoxide implementation.

[weihanglo]

Nice! Really appreciate the effort here!

We are probably not going to backports, as the backporting process involves a bit more work, and usually reserved for some regression that has no way out.

Also, given the cargo team has agreed that the dirtiness check is best effort and informational only, I think once the PR is ready we can merge it without FCP.

Thanks again, really a great job!

[Byron]

I think this is ready for a first review. Only gitoxide needs a release which is planned for the 22nd, which is when this PR becomes formally mergeable as it can point to the new release instead of the Git repository.

Especially after RustConf where I was quite apologetic and feared gitoxide could be removed again for not making progress, I thought maybe we should instead consider stabilizing the well-working parts earlier, such as in:

• nightly with opt-in
• stable with opt-out
• stable, with git2 codepath removed.

cargo package would be candidate for it as the new implementation is more correct and as fast as it can be.

What do you think?

[Byron]

I kept the structure and comments as similar as I could, compared to git2, so it's quite straightforward to compare.

[Byron]

It's unclear to me how this could possibly happen - there is no test for it either. From looking at git blame it wasn't obvious what motivated this check.
Probably nothing wrong with keeping it.

[weihanglo]

Maybe only some parts of the package is under git? Like src is a git repo.

[weihanglo]

Maybe only some parts of the package is under git? Like src is a git repo.

[weihanglo]

Out of curiosity. If we adopt the same optimization in git2 version, how the performance would look like?

[Byron]

Maybe only some parts of the package is under git? Like src is a git repo.

It seemed that what the code really was testing is if the file exists, and not if it was tracked by Git. Thus I made it an explicit 'file exists' check, which seemed strange as Cargo won't function without such a manifest. Removing the whole block doesn't fail CI either, so it's a bit unclear why it's there. There is still no tool I know that could do forensics and find everything that Git knows about it, that would certainly be interesting.

[Byron]

Out of curiosity. If we adopt the same optimization in git2 version, how the performance would look like?

The 'architecture' changes here could all be applied to the git2 implementation, with the same effects. Performance problems should be solved then.

Edit: If the team decides not to fast-track gix here, then I think the AWS team could trivially contribute a port of this to solve their problems. Of course, I vote for gix as I believe it's more correct when dealing with ignored files, which should solve a problem I am having when releasing gitoxide 😅 (it claims an ignored file is dirty, despite it being ignored).

[weihanglo]

Looks like we are mostly doing the same thing except using gitoxide. Do I understand correctly?
Apart from the dirty_files_outside_of_package_root work, have you found any other stuff to optmize?

[Byron]

The driver of these changes is the lack of single-file APIs, and I noticed no other (previously unknown) optimisation opportunities.
tldr; I don't think there are simple optimisations left, except for maybe the N^2 portion, but even there it's hard to make it show up in a profiler.

But now that I am thinking about it, there might be one: I a workspace like aws-sdk-rust I believe this code runs once for each crate, meaning there will be a full git status each time. It would certainly require more complex changes to be able to cache these between the publish calls. Each one takes a little less than a second there, so it's quite some time in aggregate.
But before that, I'd probably optimize the fetch crates stage to run only once, it takes most of the time.

[weihanglo]

While the PR is not ready to merge yet, I am kicking off the process to get a consensus on how we want to merge this.

@rfcbot fcp merge

Proposal: Switch to gix for cargo package VCS dirtiness check completely, without any fallback to libgit2.

Reasons:

• This is mostly a performance fix. Not really something fundamentally changed.
• Unlike Switch to using gitoxide by default for listing files #13696, VCS dirtiness check informational only, as the team have agreed on.
• There is already --allow-dirty users can opt out the functionality completely.

[rfcbot]

Team member @weihanglo has proposed to merge this. The next step is review by the rest of the tagged team members:

• @0xPoe
• @Eh2406
• @Muscraft
• @arlosi
• @ehuss
• @epage
• @joshtriplett
• @weihanglo

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[weihanglo]

How is the recent benchmark? Just compared with 6833aa7 against aws-sdk-rust and this PR slows down the process. Haven't checked what caused that. I assume it might be we collecting status for the entire repo for each workspace member, instead of using pathspec?

cargo package --allow-dirty --workspace --no-verify --no-metadata --offline

I ran this btw.

[weihanglo]

CARGO_LOG_PROFILE=1 cargo package --allow-dirty -p aws-sigv4 --no-verify --no-metadata --offline

master branch: 6833aa7

master-6833aa7.json

This PR d41d177

pr15534-d41d177.json

[Byron]

I didn't have time today as planned to look into this, but just had to leave a comment here nonetheless.

This is a very interesting find! On the latest main of the aws-sdk-rust repository, the status time for the whole repository is ~600ms for me (hot cache). It's strange that this takes so long in your measurements and also, that the git2 version that uses status_file on each file is fast(er) all of the sudden. It's not at all what I measured in my run.

So I will definitely look into this.
Edit: I can definitely reproduce the slowdown, awesome catch!

[Byron]

I started with hyperfine:

❯ hyperfine -w3 '/Users/byron/dev/github.com/rust-lang/cargo/target/release/cargo package --no-verify --no-metadata -p aws-smithy-runtime-api --offline' 'cargo package --no-verify --no-metadata -p aws-smithy-runtime-api --offline'
Benchmark 1: /Users/byron/dev/github.com/rust-lang/cargo/target/release/cargo package --no-verify --no-metadata -p aws-smithy-runtime-api --offline
  Time (mean ± σ):     967.5 ms ±  75.5 ms    [User: 718.7 ms, System: 3491.2 ms]
  Range (min … max):   923.2 ms … 1111.0 ms    10 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

Benchmark 2: cargo package --no-verify --no-metadata -p aws-smithy-runtime-api --offline
  Time (mean ± σ):     593.1 ms ±   7.2 ms    [User: 506.7 ms, System: 93.1 ms]
  Range (min … max):   585.3 ms … 610.4 ms    10 runs

Summary
  cargo package --no-verify --no-metadata -p aws-smithy-runtime-api --offline ran
    1.63 ± 0.13 times faster than /Users/byron/dev/github.com/rust-lang/cargo/target/release/cargo package --no-verify --no-metadata -p aws-smithy-runtime-api --offline

Even though the result is less pronounced, it's still 1.6x slower than it was before. The timings also fluctuate much more, probably a side-effect of spawning many threads for the full status check.

The profile run shows the same. Here we see how gitoxide splits up the status run into parallel operations, with the dirwalk to find untracked files being the slowest at ~700ms wallclock, the index check is parallelized and finishes in ~300ms wallclock, and finally there is the tree-index diff with ~80ms.

Also, there is no other bottleneck when looking at the actual package function.

This means in theory, if the root of the project would be provided here…

cargo/src/cargo/ops/cargo_package/vcs.rs

Line 292 in a67e9ec

.into_iter(None /* pathspec patterns */)

…one should be able to save a lot of time.

Initially I thought pathspecs weren't supported, but realised that dirty_files_outside_package_root is the reason it runs the status on the whole repository.

To me it feels inevitable to run the whole status, but the previous implementation certainly shows how it could be done. I also wonder where the performance problems are that I encountered.

My intuition here would be to eat the cost when publishing single repositories, but when multiple are done, it could do the full-repo status just once, while using a pathspec to do a per-project status instead.

I am looking at this loop:

cargo/src/cargo/ops/cargo_package/mod.rs

Lines 288 to 294 in 46ad7af

	for (pkg, cli_features) in sorted_pkgs {
	let opts = PackageOpts {
	cli_features: cli_features.clone(),
	to_package: ops::Packages::Default,
	..opts.clone()
	};
	let ar_files = prepare_archive(ws, &pkg, &opts)?;

If that was done, I'd think that a package operation per project could be about 200ms, down from 960ms or 600ms respectively.

For today I am out of time, but if you think this is acceptable (i.e. possible regression for single-packages, but speedup for multiple packages), then I'd implement it as described here. The last 'record' to package all of aws-sdk-rust is 12m (without --offline), it could probably be 10m without that extra expense.

[Kobzol]

Btw, just a walkby comment: it seemed to me like you were comparing yourself to distributed (not locally built) cargo? If that's the case, I just wanted to note that we build Cargo on CI with lto=thin, which is otherwise not configured in the Cargo repo. The LTO config should extend also to the built C/C++ code, so to libgit.

[weihanglo]

What about we do the same logic in gitoxide first, and the optimize it in follow ups? Would the same logic as the current libgit2 one be faster in gitoxide?

[Byron]

Btw, just a walkby comment: it seemed to me like you were comparing yourself to distributed (not locally built) cargo? If that's the case, I just wanted to note that we build Cargo on CI with lto=thin, which is otherwise not configured in the Cargo repo. The LTO config should extend also to the built C/C++ code, so to libgit.

Thanks for the hint, it's appreciated!
In this case, I think the effect of this would be minor as the time is spent with IO, not in heavy computation that could be more optimised in official release builds.

What about we do the same logic in gitoxide first, and the optimize it in follow ups? Would the same logic as the current libgit2 one be faster in gitoxide?

I took a stab at that and it seems to be up to 1.4x faster than the respective git2 version, in the best case. It turned out that the candidates for dirty files outside of the package root is very small, maybe even no more than 1 or 2 files. So it definitely makes sense to avoid all that extra work.
Let's see of using pathspecs works as it should.

[Byron]

Alright, now that I am done with the change, back to the previous algorithm, we are looking at 'only' 25% faster packaging in our particular example.

❯ hyperfine -w2 '/Users/byron/dev/github.com/rust-lang/cargo/target/release/cargo package --no-verify --no-metadata -p aws-smithy-runtime-api --offline' 'cargo package --no-verify --no-metadata -p aws-smithy-runtime-api --offline'
Benchmark 1: /Users/byron/dev/github.com/rust-lang/cargo/target/release/cargo package --no-verify --no-metadata -p aws-smithy-runtime-api --offline
  Time (mean ± σ):     487.4 ms ±   8.1 ms    [User: 419.8 ms, System: 103.2 ms]
  Range (min … max):   479.3 ms … 508.9 ms    10 runs

Benchmark 2: cargo package --no-verify --no-metadata -p aws-smithy-runtime-api --offline
  Time (mean ± σ):     604.6 ms ±   4.6 ms    [User: 518.0 ms, System: 93.9 ms]
  Range (min … max):   597.9 ms … 613.1 ms    10 runs

Summary
  /Users/byron/dev/github.com/rust-lang/cargo/target/release/cargo package --no-verify --no-metadata -p aws-smithy-runtime-api --offline ran
    1.24 ± 0.02 times faster than cargo package --no-verify --no-metadata -p aws-smithy-runtime-api --offline

This should address the final issue :).

[weihanglo]

Looks really good to me! I ran the benchmark again with aws-sigv4, it is at least 10% faster on a powerful Linux machine, and on macbook we gains 20% speed up!

Would you mind cleaning up commits a bit so in the future when we read the history it won't have change back and forth?

[Byron]

Thanks again for the review and the additional performance testing, @weihanglo, it's much appreciated. It's good to see it's also faster on Linux, albeit by a smaller margin.

I have squashed the PR down into 3 commits, one for the gix update, then the main implementation, and a last one for the performance optimisation hoping that it might prevent someone from thinking the previous version could be faster or simpler.

Can't wait for this to finally land :).

[weihanglo]

It's good to see it's also faster on Linux, albeit by a smaller margin.

I was using c6a.24xlarge which is fairly not representative 😬

[weihanglo]

🚀