Skip to content

Optionally use download_queue for brew install #20245

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 18, 2025
Merged

Optionally use download_queue for brew install #20245

merged 1 commit into from
Jul 18, 2025

Conversation

MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Jul 11, 2025 •
edited
Loading

Allowing using HOMEBREW_DOWNLOAD_CONCURRENCY to use the DownloadQueue for brew install by downloading and extracting bottles in parallel.

This requires some fixes in e.g. Dependency and FormulaInstaller to be able to front-load all downloads and handle parallelisation of bottle pouring.

Behaviour without HOMEBREW_DOWNLOAD_CONCURRENCY set should be unchanged.

Attestations are not handled for now and the UI should be improved before we roll this out to users.

Post-install upgrades are not yet parallelised.

Performance testing on my M1 Max MacBook Pro (that we do make -j10 on):

Benchmark 1: HOMEBREW_DOWNLOAD_CONCURRENCY=40 brew install ack wget ffmpeg
  Time (mean ± σ):     107.526 s ±  3.498 s    [User: 31.709 s, System: 65.326 s]
  Range (min … max):   105.053 s … 110.000 s    2 runs
 
Benchmark 2: HOMEBREW_DOWNLOAD_CONCURRENCY=20 brew install ack wget ffmpeg
  Time (mean ± σ):     100.833 s ±  3.007 s    [User: 31.519 s, System: 62.292 s]
  Range (min … max):   98.706 s … 102.959 s    2 runs
 
Benchmark 3: HOMEBREW_DOWNLOAD_CONCURRENCY=10 brew install ack wget ffmpeg
  Time (mean ± σ):     103.043 s ±  1.297 s    [User: 31.360 s, System: 63.587 s]
  Range (min … max):   102.126 s … 103.960 s    2 runs
 
Benchmark 4: HOMEBREW_DOWNLOAD_CONCURRENCY=2 brew install ack wget ffmpeg
  Time (mean ± σ):     139.044 s ±  3.662 s    [User: 31.711 s, System: 64.411 s]
  Range (min … max):   136.454 s … 141.633 s    2 runs
 
Benchmark 5: brew install ack wget ffmpeg
  Time (mean ± σ):     162.942 s ±  0.163 s    [User: 32.008 s, System: 57.277 s]
  Range (min … max):   162.827 s … 163.057 s    2 runs
 
Summary
  HOMEBREW_DOWNLOAD_CONCURRENCY=20 brew install ack wget ffmpeg ran
    1.02 ± 0.03 times faster than HOMEBREW_DOWNLOAD_CONCURRENCY=10 brew install ack wget ffmpeg
    1.07 ± 0.05 times faster than HOMEBREW_DOWNLOAD_CONCURRENCY=40 brew install ack wget ffmpeg
    1.38 ± 0.05 times faster than HOMEBREW_DOWNLOAD_CONCURRENCY=2 brew install ack wget ffmpeg
    1.62 ± 0.05 times faster than brew install ack wget ffmpeg

Base automatically changed from download_queue_fetch_move to main July 14, 2025 13:30
@MikeMcQuaid MikeMcQuaid force-pushed the download_queue_install branch 8 times, most recently from dc046dc to 9d06597 Compare July 17, 2025 15:24
This was referenced Jul 17, 2025
Closed
Merged
@MikeMcQuaid MikeMcQuaid force-pushed the download_queue_install branch from 9d06597 to 76bf4d2 Compare July 17, 2025 16:55
@MikeMcQuaid MikeMcQuaid marked this pull request as ready for review July 17, 2025 16:56
@MikeMcQuaid MikeMcQuaid changed the base branch from main to refactor_download_queue July 17, 2025 16:56
@MikeMcQuaid MikeMcQuaid mentioned this pull request Jul 17, 2025
Merged
carlocab
carlocab approved these changes Jul 17, 2025
View reviewed changes
Copy link
Member

@carlocab carlocab left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can't say I understand all of this, but it looks reasonable to me. Minor nits below.

@MikeMcQuaid @carlocab
Optionally use DownloadQueue for brew install
5cc6722 
Allowing using `HOMEBREW_DOWNLOAD_CONCURRENCY` to use the
`DownloadQueue` for `brew install` by downloading and extracting
bottles in parallel.

This requires some fixes in e.g. `Dependency` and `FormulaInstaller`
to be able to front-load all downloads and handle parallelisation of
bottle pouring.

Behaviour without `HOMEBREW_DOWNLOAD_CONCURRENCY` set should be
unchanged.

Attestations are not handled for now and the UI should be improved
before we roll this out to users.

Post-install upgrades are not yet parallelised.

Co-authored-by: Carlo Cabrera <github@carlo.cab>
@MikeMcQuaid MikeMcQuaid force-pushed the download_queue_install branch from 2467a1a to 5cc6722 Compare July 18, 2025 14:00
Base automatically changed from refactor_download_queue to main July 18, 2025 14:01
@MikeMcQuaid MikeMcQuaid added this pull request to the merge queue Jul 18, 2025
Merged via the queue into main with commit 0a4a299 Jul 18, 2025
34 checks passed
@MikeMcQuaid MikeMcQuaid deleted the download_queue_install branch July 18, 2025 14:39
Bo98
Bo98 reviewed Jul 20, 2025
View reviewed changes
Library/Homebrew/formula_installer.rb
Comment on lines +1393 to 1403
if (download_queue = self.download_queue) &&
(bottle = formula.bottle) &&
(manifest_resource = bottle.github_packages_manifest_resource)
download_queue.enqueue(manifest_resource)
else
begin
formula.fetch_bottle_tab(quiet: quiet)
rescue DownloadError, Resource::BottleManifest::Error
# do nothing
end
end
Copy link
Member

@Bo98 Bo98 Jul 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Need to incorporate the retry logic in

brew/Library/Homebrew/bottle.rb

Lines 123 to 132 in e804860

rescue DownloadError
raise unless fallback_on_error?
retry
rescue Resource::BottleManifest::Error
raise if @fetch_tab_retried
@fetch_tab_retried = true
resource.clear_cache
retry
since we're not calling fetch_bottle_tab anymore. Perhaps by some custom callbacks from the queue or something

The final # Do nothing rescue in the installer is also important for CI when we enable it there.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Bo98 I think this should do it? #20285

Copy link
Member

Choose a reason for hiding this comment

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

I don't see where that PR changes this

Copy link
Member Author

Choose a reason for hiding this comment

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

@Bo98 This is triggering a manual retry on bottle manifest download errors. That PR is automatically triggering a manual retry on any bottle manifest errors under the download queue.

Do you mean this logic needs reincorporated for the non-download queue case and that's missing/changed and still needs resolved beyond #20282?

Copy link
Member

@Bo98 Bo98 Jul 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oh right I see what you mean - I misunderstood there.

We do however also need a retry (with a clear_cache) if it gets a Resource::BottleManifest::Error triggered by verify_download_integrity of the bottle manifest:

brew/Library/Homebrew/resource.rb

Lines 316 to 319 in 6261551

def verify_download_integrity(_filename)
# We don't have a checksum, but we can at least try parsing it.
tab
end

Try modify a locally-cached manifest to e.g. remove all arm64 bottles and see if that works as a reproducer. (I've not retested this after the changes yet.)

Copy link
Member Author

Choose a reason for hiding this comment

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

@Bo98 gotcha, thanks, on it (and holding release on it)

Copy link
Member Author

Choose a reason for hiding this comment

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

@Bo98 Not blocking release but working on a fix for this as it only affects download queue case.

Copy link
Member Author

Choose a reason for hiding this comment

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

#20288

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bo98 Bo98 Bo98 left review comments

@carlocab carlocab carlocab approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MikeMcQuaid @Bo98 @carlocab