Add `--jobs` (parallel git fetch)

crystal-lang / shards

Dependency manager for the Crystal language

Other

763 stars 100 forks source link

Add `--jobs` (parallel git fetch) #539

Closed m-o-e closed 2 years ago

m-o-e commented 2 years ago

Resolves #371 Resolves #239

This PR parallelizes git fetching which gives me a ~40% speed-up for shards install on average.

$ git clone https://github.com/amberframework/amber.git
$ cd amber

$ time shards --jobs=1
18.579s total

$ time shards --jobs=8
10.143s total

Only the fetching is parallelized, post-install still runs in serial. I've experimented with parallelizing that too but the speed gains were negligible and I'm not sure if it's always safe.
It's only implemented for git so far (no hg)
No test-coverage, yet! Although parallelism is enabled during test runs, the existing specs don't seem to actually exercise it.

straight-shoota commented 2 years ago

It would be great to have specs for parallel fetch.

m-o-e commented 2 years ago

It would be great to have specs for parallel fetch.

Agree.

Now that it's enabled in spec it should get plenty coverage. Do you think that is sufficient or should there be an isolated test? (not sure how to create a meaningful one tbh, ideas welcome 🤔)

m-o-e commented 2 years ago

After using this PR-version for a while I realized it would only parallelize when a shard.lock exists, but not for the initial run after cloning a repo without that file.

This is now fixed.

(had missed it in my previous testing because I'd muck with shard.yml, delete lib, but never the shard.lock...)

carlhoerberg commented 2 years ago

I purpose that you rename the argument to -j/--jobs, like make, bundler and #545 uses.

m-o-e commented 2 years ago

Good idea @carlhoerberg!

Renamed in https://github.com/crystal-lang/shards/pull/539/commits/bac393b6cdcc19af02b84fe66027fdffad8421cb

Fwiw, I left the help-screen as Number of git fetches to perform in parallel (default: 8) to keep it clear what it actually does. If anyone would prefer a different text just let me know. :)

I'm torn on whether -j should also be added, left it out here for now. (I guess for consistency with make/bundler we could add it - but otoh I don't think anyone would blindly rely on -j to be present, and it may stand out a bit weirdly as most of the other flags don't have a short version)