Repository navigation
Download missing dist archives in parallel - #1233
Conversation
438c8e5 to
9b41b2f
Compare
|
This sounds like a good improvement in theory. I am just curious, what will happen if the platform/system does not support concurrency? |
There was a problem hiding this comment.
🟡 Changes recommended
Target-directory creation outside the per-package try/catch lets a single package’s setup failure abort the entire build despite --skip-errors, rather than skipping that package.
1 open issue
What changed in this PR
This PR speeds up archive generation by downloading missing ZIP and TAR distributions concurrently when rearchive is disabled, using Composer’s HTTP downloader.
Changes:
- Prefetches missing archives and shares path and download helpers with the sequential archive loop.
- Adds tests for download scheduling, existing archives, retries, and error handling.
- Declares
react/promiseas a direct dependency and updates lock metadata.
| File | Description |
|---|---|
tests/Builder/ArchiveBuilderTest.php |
Adds coverage for concurrent scheduling and download failure handling. |
src/Builder/ArchiveBuilder.php |
Adds parallel prefetching and extracts shared archive helpers. |
composer.lock |
Updates the content hash and prefer-lowest metadata. |
composer.json |
Adds the direct react/promise dependency. |
Review effort: Balanced
Thanks. Composer's |
Yes that is what I kind of assumed. But since we do it up front now.. does that make sense? |
I assume you refer to ArchiveBuilder, line 56? I think you're right, I'll check how this can be improved without calling Composer internals. |
Yes, correct. Can we perhaps detect if concurrency is supported or not and branch accordingly in the code? |
Composer itself doesn't offer a public way to detect this. The only check is Instead I aligned the behavior with composer install, which also downloads everything up front regardless of curl (InstallationManager: "execute all downloads first"). Without curl, the downloads simply run one after another before archiving starts. In b91eb94 I also removed the retry, so each download is attempted exactly once and errors are still reported per package as before. If you'd still prefer explicit branching, I can mirror Composer's check ( |
With `rearchive` disabled, dist archives of type zip or tar are downloaded as is. Each download was awaited before the next one started, so a build creating thousands of archives spent most of its time waiting for single HTTP requests. `ArchiveBuilder::dump()` now starts the downloads of all missing dist archives up front and waits for them together, letting Composer's `HttpDownloader` run them concurrently (12 by default, configurable via `COMPOSER_MAX_PARALLEL_HTTP`). The archive loop then finds the files in place. Failed downloads are ignored at this stage and retried by the loop, so error handling and `--skip-errors` behave as before. The path and download logic of `archive()` is extracted into helper methods shared by both steps. `ArchiveBuilder` uses `react/promise` natively, therefore it is added as a direct dependency.
`prefetchDistArchives()` created the target directory of every package, even with `rearchive` enabled where nothing is prefetched, and did so outside the per-package try/catch of `dump()`. A package whose target directory could not be created aborted the whole build despite `--skip-errors`. The same applied to downloads failing synchronously, e.g. for packages without a dist URL. The prefetch no longer creates directories, as `downloadDistArchive()` already ensures the directory of the archives it downloads. Synchronous download errors are ignored like rejected downloads, leaving reporting and skipping them to `archive()`. Also removes a blank line flagged by php-cs-fixer.
Failed prefetch downloads are no longer retried by archive(). Instead, the prefetched download is passed to archive(), which reports its error for the respective package as before. Without curl, Composer's HttpDownloader runs the prefetch downloads one after another, like `composer install` does. As each download is attempted once, this matches the number of downloads before the prefetch was introduced.
b91eb94 to
4c9bde4
Compare
|
No, that is more work than necessary perhaps. This is good enough. Thanks for contributing! |
There was a problem hiding this comment.
🟡 Changes recommended
Archive prefetching can leave temporary downloads behind when destination preparation or moves fail and trigger duplicate downloads when multiple versions resolve to the same archive path.
2 open issues
1 resolved since last review
Review effort: Balanced
Versions pointing to the same commit, such as a tag and a branch, resolve to the same archive path. Prefetching now downloads each path once and shares the download between these packages, as the sequential loop did before. The temporary download directory is now also removed if moving the archive into place fails, or if the download fails immediately. The test downloads now only complete while the loop waits, like with Composer's curl downloader, so the tests cover concurrent downloads.
There was a problem hiding this comment.
🟡 Changes recommended
Packages sharing an archive path inherit a failed download promise without trying their own distinct URLs, so --skip-errors can skip packages whose downloads would succeed.
1 open issue
2 resolved since last review
Review effort: Balanced
|
Sorry for re-checking every time; but I don't really have time myself to dive deeper into your contribution 😅 |
That's totally fine. The findings seem to be valid, so no need to be sorry 😄. |
Packages sharing an archive path can have different dist URLs. If the download from one URL fails, the next URL is now tried, so a working URL is not skipped because of a failing one. Further URLs are only tried after the previous download failed, so the archive is never written concurrently, and each URL is still attempted once.

With
rearchivedisabled, dist archives of type zip or tar are downloaded as is. Each download was awaited before the next one started, so a build creating thousands of archives spent most of its time waiting for single HTTP requests.ArchiveBuilder::dump()now starts the downloads of all missing dist archives up front and waits for them together, letting Composer'sHttpDownloaderrun them concurrently (12 by default, configurable viaCOMPOSER_MAX_PARALLEL_HTTP). The archive loop then finds the files in place. Failed downloads are ignored at this stage and retried by the loop, so error handling and--skip-errorsbehave as before.The path and download logic of
archive()is extracted into helper methods shared by both steps.ArchiveBuilderusesreact/promisenatively, therefore it is added as a direct dependency.The benchmark was executed on Apple M3 Pro, 36 GB RAM, SSD with APFS, and PHP 8.5.12 installed. Each run downloaded 700 packages.