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

Implement concurrent downloads in brew fetch. #17756

Merged
merged 28 commits into from
Sep 9, 2024

Conversation

reitermarkus
Copy link
Member

@reitermarkus reitermarkus commented Jul 15, 2024

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

$ hyperfine 'brew fetch --force --build-from-source azure-cli --concurrency 1'
Benchmark 1: brew fetch --force --build-from-source azure-cli --concurrency 1
  Time (mean ± σ):     63.889 s ±  2.812 s    [User: 16.251 s, System: 14.726 s]
  Range (min … max):   61.079 s … 70.527 s    10 runs
$ hyperfine 'brew fetch --force --build-from-source azure-cli --concurrency 2'
Benchmark 1: brew fetch --force --build-from-source azure-cli --concurrency 2
  Time (mean ± σ):     35.500 s ±  3.112 s    [User: 15.205 s, System: 14.819 s]
  Range (min … max):   30.537 s … 40.762 s    10 runs
concurrent-downloads.mov

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Makes sense to me so far! Some questions:

  • what made you decide on concurrent-ruby and whirly gems?
  • does this use threads or ractors?
  • does this slow down/add additional requires to a brew help command at all?

Thanks!

Library/Homebrew/api/download.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/fetch.rb Outdated Show resolved Hide resolved
@reitermarkus
Copy link
Member Author

what made you decide on concurrent-ruby and whirly gems?

concurrent-ruby because I have been using it for parallel brew fetch in my dotfiles for years, whirly to get at least some sort of progress indicator. The end goal is to have a progress bar, but for that all download strategies need to support progress updates first.

does this use threads or ractors?

Threads. From reading ruby-concurrency/concurrent-ruby#899, it seems ractors are quite limited in what they can do. In any case, network is the bottleneck here.

does this slow down/add additional requires to a brew help command at all?

No, the additional requires are only called when the thread pool is created, i.e. only if the downloads are actually started.

@reitermarkus
Copy link
Member Author

Improved output which shows all concurrent downloads at once.

concurrent-downloads.mov

@reitermarkus reitermarkus force-pushed the concurrent-downloads branch 3 times, most recently from ed41a39 to f74113f Compare July 16, 2024 01:17
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks great! A few tiny nits here and then can self-merge. Nice work @reitermarkus!

Library/Homebrew/cmd/fetch.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/fetch.rb Outdated Show resolved Hide resolved
Library/Homebrew/download_queue.rb Show resolved Hide resolved
@MikeMcQuaid
Copy link
Member

Looks good when comments addressed.

@MikeMcQuaid MikeMcQuaid marked this pull request as draft August 15, 2024 08:52
@MikeMcQuaid
Copy link
Member

@reitermarkus converted to draft until you're ready to merge

@reitermarkus reitermarkus force-pushed the concurrent-downloads branch 3 times, most recently from 6cca97d to f9726d6 Compare August 15, 2024 15:40
@MikeMcQuaid
Copy link
Member

@reitermarkus do you have any ETA on this being mergeable?

@reitermarkus reitermarkus force-pushed the concurrent-downloads branch 4 times, most recently from 255b99a to 0f0e991 Compare September 4, 2024 20:50
@reitermarkus reitermarkus force-pushed the concurrent-downloads branch 4 times, most recently from eb5c035 to 90ced30 Compare September 4, 2024 21:55
Copy link
Member

@Bo98 Bo98 left a comment

Choose a reason for hiding this comment

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

Looks ok though I have a few questions that might have been from me reading things in the wrong order as there's a few different things going on.

Library/Homebrew/downloadable.rb Show resolved Hide resolved
Library/Homebrew/downloadable.rb Show resolved Hide resolved
Library/Homebrew/download_strategy.rb Show resolved Hide resolved
Library/Homebrew/formula.rb Outdated Show resolved Hide resolved
Library/Homebrew/formula.rb Outdated Show resolved Hide resolved
Library/Homebrew/formula.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/fetch.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/fetch.rb Show resolved Hide resolved
@MikeMcQuaid
Copy link
Member

Probable makes sense to merge this and open an issue for the things still missing.

@reitermarkus Sounds good. Could you address @Bo98's comments, consider adding an opt-in undocumented environment variable (HOMEBREW_FETCH_PARALLEL or something, could do in a follow-up PR instead) and then self-merge?

@MikeMcQuaid
Copy link
Member

consider adding an opt-in undocumented environment variable (HOMEBREW_FETCH_PARALLEL or something, could do in a follow-up PR instead)

I forgot: you can do this for switch but not flag so: let's hold off for now.


@reitermarkus good to merge when @Bo98 is ✅
@Bo98 given this is "feature flagged": please mainly just check for if you have concerns about breaking existing functionality. Any issues with the new functionality we can fix forward.

@reitermarkus
Copy link
Member Author

I forgot: you can do this for switch but not flag so: let's hold off for now.

It's hidden for now anyways, i.e. undocumented and unsupported.

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Tested brew fetch locally with and without --concurrency. Seems to work well.

Library/Homebrew/cmd/fetch.rb Show resolved Hide resolved
@MikeMcQuaid MikeMcQuaid merged commit c8e8aa5 into Homebrew:master Sep 9, 2024
29 checks passed
@MikeMcQuaid
Copy link
Member

We've just made a new tag so let's try to land this as it's now blocking several other PRs.

Please feel free to make follow-up PRs to clean this up a bit, anyone/everyone.

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.

5 participants