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

Disable git GC before fetch #194

Merged
merged 4 commits into from
Dec 19, 2022
Merged

Disable git GC before fetch #194

merged 4 commits into from
Dec 19, 2022

Conversation

ofalvai
Copy link
Contributor

@ofalvai ofalvai commented Dec 19, 2022

Checklist

  • I've read and followed the Contribution Guidelines
  • step.yml and README.md is updated with the changes (if needed)

Version

Requires a MAJOR/MINOR/PATCH version update

Context

It's probably a good idea to disable git GC to make step run times more deterministic. See the investigation details section for numbers.

Changes

Run git config gc.auto 0 before the fetch commands.

Investigation details

I created a Go benchmark for measuring the difference. This test case fetches the https://github.com/androidx/androidx repo main branch, which has quite a long history. It's not a super scientific test as it includes a lot of network calls, but at least it shows that this change doesn't slow us down:

❯ go test -bench=BenchmarkBranchShallowFetch -run=^# -benchtime=3x
goos: darwin
goarch: arm64
pkg: github.com/bitrise-steplib/steps-git-clone/gitclone
BenchmarkBranchShallowFetch/jobs=10,disableGC=false-10                 3        6757783875 ns/op
BenchmarkBranchShallowFetch/jobs=10,disableGC=true-10                  3        6427483806 ns/op

The reason I'm not including the benchmark in this PR is that it required uncommenting a few parts of the step code to make it work.

Decisions

@ofalvai ofalvai requested a review from hisaac December 19, 2022 10:36
Copy link
Contributor

@hisaac hisaac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, good thinking. I hadn't heard of git gc before this.

@ofalvai ofalvai merged commit a50bdc5 into next Dec 19, 2022
@ofalvai ofalvai deleted the ACI-189-disable-gc branch December 19, 2022 15:03
@Grayson
Copy link

Grayson commented Dec 19, 2022

Huh. I'm surprised that we're doing enough locally to trigger any actual auto collection. I'm assuming that this merely stops git from doing the checks to see if a GC is necessary. Do we know if it pre-empts those checks or actually stops gc from happening?

@ofalvai
Copy link
Contributor Author

ofalvai commented Dec 20, 2022

I couldn't really prove that GC kicks in because git maintenance run --auto just returned an empty output for me in all my tests. But I also don't want this ever to run and distort our checkout time benchmarks in random ways, that's why I think it's better to include this in the step.

Also, based on the docs, applying this config not only stops GC to kick in, but stops collecting the metrics that might trigger the GC:

Setting this to 0 disables not only automatic packing based on the number of loose objects, but any other heuristic git gc --auto will otherwise use to determine if there’s work to do, such as gc.autoPackLimit.

ofalvai added a commit that referenced this pull request Jan 3, 2023
* Disable git GC before fetch

* Print failed command output too

* Reorder commands

* Use standard runner for the git config command
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants