Skip to content

Optionally use download_queue for brew install#20245

Merged
MikeMcQuaid merged 1 commit into
mainfrom
download_queue_install
Jul 18, 2025
Merged

Optionally use download_queue for brew install#20245
MikeMcQuaid merged 1 commit into
mainfrom
download_queue_install

Conversation

@MikeMcQuaid

@MikeMcQuaid MikeMcQuaid commented Jul 11, 2025

Copy link
Copy Markdown
Member

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
@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

@carlocab carlocab left a comment

Copy link
Copy Markdown
Member

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.

Comment thread Library/Homebrew/download_queue.rb
Comment thread Library/Homebrew/formula_installer.rb
Comment thread Library/Homebrew/formula_installer.rb
Comment thread Library/Homebrew/install.rb Outdated
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
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

@Bo98 Bo98 Jul 20, 2025

Copy link
Copy Markdown
Member

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

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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?

@Bo98 Bo98 Jul 21, 2025

Copy link
Copy Markdown
Member

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:

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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