Implement concurrent downloads in `brew fetch`. #17756

reitermarkus · 2024-07-15T14:22:34Z

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

MikeMcQuaid

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

Library/Homebrew/cmd/fetch.rb

reitermarkus · 2024-07-15T16:08:12Z

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 · 2024-07-15T20:36:58Z

Improved output which shows all concurrent downloads at once.

concurrent-downloads.mov

MikeMcQuaid

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

Library/Homebrew/cmd/fetch.rb

Library/Homebrew/download_queue.rb

MikeMcQuaid · 2024-08-02T10:28:32Z

Looks good when comments addressed.

MikeMcQuaid · 2024-08-15T08:53:12Z

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

MikeMcQuaid · 2024-09-04T07:41:27Z

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

# Conflicts: # Library/Homebrew/Gemfile.lock

reitermarkus · 2024-09-04T20:59:18Z

@MikeMcQuaid, just rebased and technically it should be mergeable, but I'd consider it an alpha-quality PoC, given there are many things still missing.

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

Bo98

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

Bo98 · 2024-09-05T03:21:49Z

Library/Homebrew/downloadable.rb

+
+ sig { returns(String) }
+ def download_type
+ T.must(self.class.name&.split("::")&.last).gsub(/([[:lower:]])([[:upper:]])/, '\1 \2').downcase


This is very gnarly. May be worth adding a comment example.

We only seem to not be overriding this in one place so maybe it's worth just making it abstract?

only seem to not be overriding this in one place

Actually seven places, i.e. every subclass of Resource, Resource itself, and Bottle.

Library/Homebrew/download_strategy.rb

Library/Homebrew/formula.rb

Library/Homebrew/cmd/fetch.rb

MikeMcQuaid · 2024-09-05T09:04:06Z

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?

Co-authored-by: Bo Anderson <[email protected]>

Library/Homebrew/formula.rb

Library/Homebrew/cmd/fetch.rb

MikeMcQuaid · 2024-09-06T07:29:37Z

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 · 2024-09-07T13:00:35Z

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.

carlocab

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

MikeMcQuaid reviewed Jul 15, 2024

View reviewed changes

Library/Homebrew/api/download.rb Outdated Show resolved Hide resolved

Library/Homebrew/cmd/fetch.rb Outdated Show resolved Hide resolved

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

MikeMcQuaid approved these changes Jul 25, 2024

View reviewed changes

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

reitermarkus force-pushed the concurrent-downloads branch from 9075145 to 8bb8b5a Compare July 30, 2024 19:38

MikeMcQuaid approved these changes Aug 2, 2024

View reviewed changes

reitermarkus force-pushed the concurrent-downloads branch from 12143e0 to bf4acae Compare August 14, 2024 20:01

MikeMcQuaid marked this pull request as draft August 15, 2024 08:52

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

reitermarkus force-pushed the concurrent-downloads branch 4 times, most recently from 255b99a to 0f0e991 Compare September 4, 2024 20:50

reitermarkus added 9 commits September 4, 2024 22:54

Vendor Gems.

4c29d4c

# Conflicts: # Library/Homebrew/Gemfile.lock

Implement concurrent downloads.

b297be7

Convert Downloadable to a module.

b6d529d

Fix type signatures.

f41d251

Color status.

b55528b

Fix --force flag.

601134b

Don't require everything.

b353e9c

Move require.

3a51f55

Remove Whirly and show concurrent downloads.

6434533

reitermarkus added 2 commits September 4, 2024 22:54

Add method for moving cursor up and to the beginning.

9fd4c97

Add FIXME comment.

231ac72

reitermarkus force-pushed the concurrent-downloads branch from 0f0e991 to 231ac72 Compare September 4, 2024 20:54

reitermarkus marked this pull request as ready for review September 4, 2024 20:59

reitermarkus force-pushed the concurrent-downloads branch 4 times, most recently from eb5c035 to 90ced30 Compare September 4, 2024 21:55

Fix wrong stage method being called.

fb0bf3b

reitermarkus force-pushed the concurrent-downloads branch from 90ced30 to fb0bf3b Compare September 4, 2024 22:21

Bo98 reviewed Sep 5, 2024

View reviewed changes

reitermarkus and others added 3 commits September 5, 2024 17:14

Fix handling for --retry flag.

c61713b

Comment deprecations.

123a2ac

Co-authored-by: Bo Anderson <[email protected]>

Fix wrong argument passing.

846cf25

reitermarkus commented Sep 5, 2024

View reviewed changes

Library/Homebrew/formula.rb Outdated Show resolved Hide resolved

reitermarkus added 2 commits September 5, 2024 17:20

Fix wrong argument passing.

a970065

Readd Pourable to LocalBottleDownloadStrategy.

60cb810

reitermarkus commented Sep 5, 2024

View reviewed changes

Library/Homebrew/cmd/fetch.rb Outdated Show resolved Hide resolved

Fix code style.

69a04bd

MikeMcQuaid requested a review from Bo98 September 6, 2024 07:13

MikeMcQuaid reviewed Sep 6, 2024

View reviewed changes

Library/Homebrew/cmd/fetch.rb Show resolved Hide resolved

MikeMcQuaid force-pushed the concurrent-downloads branch from 4761817 to 69a04bd Compare September 6, 2024 07:28

Ensure thread-pool shutdown.

b68ee41

carlocab approved these changes Sep 7, 2024

View reviewed changes

reitermarkus mentioned this pull request Sep 7, 2024

Tracking Issue: Concurrent Downloads #18278

Open

7 tasks

carlocab approved these changes Sep 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement concurrent downloads in `brew fetch`. #17756

Implement concurrent downloads in `brew fetch`. #17756

reitermarkus commented Jul 15, 2024 •

edited

Loading

MikeMcQuaid left a comment

reitermarkus commented Jul 15, 2024

reitermarkus commented Jul 15, 2024

MikeMcQuaid left a comment

MikeMcQuaid commented Aug 2, 2024

MikeMcQuaid commented Aug 15, 2024

MikeMcQuaid commented Sep 4, 2024

reitermarkus commented Sep 4, 2024 •

edited

Loading

Bo98 left a comment

Bo98 Sep 5, 2024

reitermarkus Sep 7, 2024 •

edited

Loading

MikeMcQuaid commented Sep 5, 2024

MikeMcQuaid commented Sep 6, 2024

reitermarkus commented Sep 7, 2024

carlocab left a comment

Implement concurrent downloads in brew fetch. #17756

Are you sure you want to change the base?

Implement concurrent downloads in brew fetch. #17756

Conversation

reitermarkus commented Jul 15, 2024 • edited Loading

MikeMcQuaid left a comment

Choose a reason for hiding this comment

reitermarkus commented Jul 15, 2024

reitermarkus commented Jul 15, 2024

MikeMcQuaid left a comment

Choose a reason for hiding this comment

MikeMcQuaid commented Aug 2, 2024

MikeMcQuaid commented Aug 15, 2024

MikeMcQuaid commented Sep 4, 2024

reitermarkus commented Sep 4, 2024 • edited Loading

Bo98 left a comment

Choose a reason for hiding this comment

Bo98 Sep 5, 2024

Choose a reason for hiding this comment

reitermarkus Sep 7, 2024 • edited Loading

Choose a reason for hiding this comment

MikeMcQuaid commented Sep 5, 2024

MikeMcQuaid commented Sep 6, 2024

reitermarkus commented Sep 7, 2024

carlocab left a comment

Choose a reason for hiding this comment

Implement concurrent downloads in `brew fetch`. #17756

Implement concurrent downloads in `brew fetch`. #17756

reitermarkus commented Jul 15, 2024 •

edited

Loading

reitermarkus commented Sep 4, 2024 •

edited

Loading

reitermarkus Sep 7, 2024 •

edited

Loading