gitoxide progress bar fixes #11800

Byron · 2023-03-04T12:53:34Z

This PR makes the progress bar shown when fetching git repositories with -Zgitoxide feel more similar to the git2 implementation.

The main difference is that it separates the indexing and delta-resolution stage at 50%, instead of at ~33%.
Furthermore it 'cools' the hot loop that was dealing with progress display, which previously used one whole core.

Here is how it looks now:

Videos

how it looks with git2 https://user-images.githubusercontent.com/63622/222902630-9cc5cd60-beff-4a70-a791-c264aec46e3b.mov
gitoxide before the fix: https://user-images.githubusercontent.com/63622/222902777-1272982c-8c35-4b41-8350-80ac0f2296de.mov
and gitoxide after the fix: https://user-images.githubusercontent.com/63622/222902878-9f35d9a9-9603-49d4-b751-e05884684d52.mov

rustbot · 2023-03-04T12:53:39Z

r? @epage

(rustbot has picked a reviewer for you, use r? to override)

The `git2` implementation can leverage that `git2` provides a consistent view on the objects to be index, so it looks like 33% of the time is spent receiving objects, and the rest of the time is used resolving them. In `gitoxide`, there are two distinct phases and these are exposed by the way we obtain progress for two separate phases. We have to do some math to renormalize those to a single, continuous progress by mapping the values for 'amount of objects' to the first half and second half of the progress bar respectively. This has the advantage of having the first phase (receiving objects) end at 50% and the second phase (resolving deltas) at 100%.

Previously it actually was very hot and needed an entire CPU for itself due to constant querying of progress information. Now it's slowed down by consistently sleeping for a short amount of time. This time should be short enough to not let the progress bar hold up the overall progress of the fetch operation, hence the 10ms sleep time, reducing the worst-case hold-up to 10ms.

weihanglo

It looks fantastic! Thanks a lot!

weihanglo · 2023-03-04T17:41:38Z

Cargo.toml

-gix = { version = "0.38.0", default-features = false, features = ["blocking-http-transport-curl", "progress-tree"] }
-gix-features-for-configuration-only = { version = "0.27.0", package = "gix-features", features = [ "parallel" ] }
+gix = { version = "0.39.0", default-features = false, features = ["blocking-http-transport-curl", "progress-tree"] }
+gix-features-for-configuration-only = { version = "0.28.0", package = "gix-features", features = [ "parallel" ] }


Forgot to ask in the previous PR review. Could you give me a pointer to how this configuration works?

The gix-features crate contains all parts of an implementation that are changeable at compile-time with cargo features. Plumbing crates pull it in and if necessary, enable 'base' versions of the capabilities they need. Those are always pure-Rust implementations of algorithms like zlib or SHA1. The gix-features crate also contains primitives for parallelism and utilities to parallelize work.

The gix crate as entry-point for application developers now pulls in gix-features to enable typical features that add on top of the baseline, like enabling actual parallelism with threads, or better performing versions of core algorithms. This works because cargo-feature resolution will compile gix-features only once and aggregate all features set by all crates that use it.

Now, cargo has disabled default features for gix and only re-enables a few ones. None of the features provided by the gix-features crate are 'forwarded' in the gix crate to avoid duplication, so cargo has to pull it in itself to get a chance to set the features it needs. For now, this is only parallel to get parallel computation for everything. but it could also enable faster SHA1 or zlib implementations, but that would pull in more C - right now gitoxide is configured to be 'pure Rust' which should be most portable.

You can try it yourself by commenting out the gix-features-for-configuration-only … line and fetch the crates index - it will be single-threaded then and quite a bit longer. It will still be faster than the git2 version though as it's optimal, and doesn't do any unnecessary computation. It's quite neat to see that the gix_feature::parallel functions also exist in API-compatible serial forms that work the same - those kick in when parallel is not set.

I think @epage called it a 'cargo-features-hack', and that's certainly a fitting name as well 😁.

Thank you so much! I didn't expect such a comprehensive explanation 😆. Really really useful!

weihanglo · 2023-03-04T17:42:58Z

@bors r+

bors · 2023-03-04T17:43:00Z

📌 Commit 789efe3 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-03-04T17:43:08Z

⌛ Testing commit 789efe3 with merge e5e5d21...

bors · 2023-03-04T18:31:23Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing e5e5d21 to master...

23 commits in 9880b408a3af50c08fab3dbf4aa2a972df71e951..c1334b059c6dcceab3c10c81413f79bb832c8d9d 2023-02-28 19:39:39 +0000 to 2023-03-07 19:21:50 +0000 - Add `CARGO_PKG_README` (rust-lang/cargo#11645) - path dependency: fix cargo-util version (rust-lang/cargo#11807) - Adding display of which target failed to compile (rust-lang/cargo#11636) - Fix `CARGO_CFG_` vars for configs defined both with and without value (rust-lang/cargo#11790) - Breaking endless loop on cyclic features in added dependency in cargo-add (rust-lang/cargo#11805) - Enhance the doc of timing report with graphs (rust-lang/cargo#11798) - Make `sparse` the default protocol for crates.io (rust-lang/cargo#11791) - Use sha2 to calculate SHA256 (rust-lang/cargo#11795) - gitoxide progress bar fixes (rust-lang/cargo#11800) - Check publish_to_alt_registry publish content (rust-lang/cargo#11799) - chore: fix missing files in autolabel trigger_files (rust-lang/cargo#11797) - chore: Update base64 (rust-lang/cargo#11796) - Fix some doc typos (rust-lang/cargo#11794) - chore(ci): Enforce cargo-deny in CI (rust-lang/cargo#11761) - Some cleanup for unstable docs (rust-lang/cargo#11793) - gitoxide integration: fetch (rust-lang/cargo#11448) - patch can conflict on not activated packages (rust-lang/cargo#11770) - fix(toml): Provide a way to show unused manifest keys for dependencies (rust-lang/cargo#11630) - Improve error for missing crate in --offline mode for sparse index (rust-lang/cargo#11783) - feat(resolver): `-Zdirect-minimal-versions` (rust-lang/cargo#11688) - feat: Use test name for dir when running tests (rust-lang/cargo#11738) - Jobserver cleanup (rust-lang/cargo#11764) - Fix help string for "--charset" option of "cargo tree" (rust-lang/cargo#11785) Note that some 3rd-party licensing allowed list changed due to the introducion of `gix` dependency

Update cargo 25 commits in 9880b408a3af50c08fab3dbf4aa2a972df71e951..7d3033d2e59383fd76193daf9423c3d141972a7d 2023-02-28 19:39:39 +0000 to 2023-03-08 17:05:08 +0000 - Revert "rust-lang/cargo#11738" - Use test name for dir when running tests (rust-lang/cargo#11812) - Update CHANGELOG for 1.68 backports (rust-lang/cargo#11810) - Add `CARGO_PKG_README` (rust-lang/cargo#11645) - path dependency: fix cargo-util version (rust-lang/cargo#11807) - Adding display of which target failed to compile (rust-lang/cargo#11636) - Fix `CARGO_CFG_` vars for configs defined both with and without value (rust-lang/cargo#11790) - Breaking endless loop on cyclic features in added dependency in cargo-add (rust-lang/cargo#11805) - Enhance the doc of timing report with graphs (rust-lang/cargo#11798) - Make `sparse` the default protocol for crates.io (rust-lang/cargo#11791) - Use sha2 to calculate SHA256 (rust-lang/cargo#11795) - gitoxide progress bar fixes (rust-lang/cargo#11800) - Check publish_to_alt_registry publish content (rust-lang/cargo#11799) - chore: fix missing files in autolabel trigger_files (rust-lang/cargo#11797) - chore: Update base64 (rust-lang/cargo#11796) - Fix some doc typos (rust-lang/cargo#11794) - chore(ci): Enforce cargo-deny in CI (rust-lang/cargo#11761) - Some cleanup for unstable docs (rust-lang/cargo#11793) - gitoxide integration: fetch (rust-lang/cargo#11448) - patch can conflict on not activated packages (rust-lang/cargo#11770) - fix(toml): Provide a way to show unused manifest keys for dependencies (rust-lang/cargo#11630) - Improve error for missing crate in --offline mode for sparse index (rust-lang/cargo#11783) - feat(resolver): `-Zdirect-minimal-versions` (rust-lang/cargo#11688) - feat: Use test name for dir when running tests (rust-lang/cargo#11738) - Jobserver cleanup (rust-lang/cargo#11764) - Fix help string for "--charset" option of "cargo tree" (rust-lang/cargo#11785) --- ~~This update is primarily for making rust-lang/cargo#11630 into 1.69~~ (will file a beta backport then). However, just look into the licenses and dependencies permitted list, it looks a bit unfortunate but inevitable I guess? r? `@ehuss` cc `@Muscraft`

rustbot assigned epage Mar 4, 2023

rustbot added A-git Area: anything dealing with git S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 4, 2023

Byron added 2 commits March 4, 2023 14:20

Byron force-pushed the progress-bar-fixes branch from 930ffbe to 789efe3 Compare March 4, 2023 13:20

weihanglo approved these changes Mar 4, 2023

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 4, 2023

bors merged commit e5e5d21 into rust-lang:master Mar 4, 2023

Byron deleted the progress-bar-fixes branch March 4, 2023 18:58

weihanglo mentioned this pull request Mar 6, 2023

Update cargo rust-lang/rust#108665

Merged

weihanglo added the Z-gitoxide Nightly: gitoxide integration label Mar 9, 2023

weihanglo mentioned this pull request Mar 9, 2023

Tracking Issue for -Zgitoxide #11813

Open

15 tasks

ehuss added this to the 1.70.0 milestone Mar 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gitoxide progress bar fixes #11800

gitoxide progress bar fixes #11800

Byron commented Mar 4, 2023 •

edited

Loading

rustbot commented Mar 4, 2023

weihanglo left a comment

weihanglo Mar 4, 2023

Byron Mar 4, 2023

weihanglo Mar 4, 2023

weihanglo commented Mar 4, 2023

bors commented Mar 4, 2023

bors commented Mar 4, 2023

bors commented Mar 4, 2023

gitoxide progress bar fixes #11800

gitoxide progress bar fixes #11800

Conversation

Byron commented Mar 4, 2023 • edited Loading

Videos

rustbot commented Mar 4, 2023

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo Mar 4, 2023

Choose a reason for hiding this comment

Byron Mar 4, 2023

Choose a reason for hiding this comment

weihanglo Mar 4, 2023

Choose a reason for hiding this comment

weihanglo commented Mar 4, 2023

bors commented Mar 4, 2023

bors commented Mar 4, 2023

bors commented Mar 4, 2023

Byron commented Mar 4, 2023 •

edited

Loading