Skip to content

Parallelize ObjectStorageProvider downloads - #2088

Merged
Billy O'Neal (BillyONeal) merged 2 commits into
microsoft:mainfrom
BillyONeal:parallel-binarycaching
Aug 5, 2026
Merged

Parallelize ObjectStorageProvider downloads#2088
Billy O'Neal (BillyONeal) merged 2 commits into
microsoft:mainfrom
BillyONeal:parallel-binarycaching

Conversation

@BillyONeal

Copy link
Copy Markdown
Member

This is #1485 / #1392 now that we have the DiagnosticContext infrastructure to make it not painful.

Run up to eight external object-storage downloads concurrently while respecting lower VCPKG_MAX_CONCURRENCY values. Each worker writes to its own result slot and FullyBufferedDiagnosticContext; after all workers finish, diagnostics are replayed on the calling thread in action order, avoiding concurrent console I/O.

Add a bounded execute_in_parallel overload without changing existing callers, plus regression coverage for its concurrency ceiling.

This is microsoft#1485 / microsoft#1392 now that we have the DiagnosticContext infrastructure to make it not painful.

Run up to eight external object-storage downloads concurrently while respecting lower VCPKG_MAX_CONCURRENCY values. Each worker writes to its own result slot and FullyBufferedDiagnosticContext; after all workers finish, diagnostics are replayed on the calling thread in action order, avoiding concurrent console I/O.

Add a bounded execute_in_parallel overload without changing existing callers, plus regression coverage for its concurrency ceiling.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR parallelizes object-storage binary cache downloads in ObjectStorageProvider by introducing a bounded-concurrency execute_in_parallel overload, while preserving ordered diagnostics by buffering them per worker and replaying on the caller thread.

Changes:

  • Parallelize ObjectStorageProvider::acquire_zips() downloads with a fixed upper bound (8) and per-action buffered diagnostics.
  • Add a execute_in_parallel(work_count, max_concurrency, ...) overload (keeping the existing signature as a wrapper).
  • Add a regression test to ensure the new overload respects the provided concurrency ceiling.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/vcpkg/binarycaching.cpp Runs object-storage downloads concurrently and replays diagnostics deterministically after completion.
include/vcpkg/base/parallel-algorithms.h Adds a max-concurrency overload for execute_in_parallel and keeps the legacy overload.
src/vcpkg-test/parallel-algorithms.cpp Adds test coverage validating the max-concurrency limit is honored.

Comment thread include/vcpkg/base/parallel-algorithms.h
Copilot AI review requested due to automatic review settings July 29, 2026 00:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

include/vcpkg/base/parallel-algorithms.h:145

  • On non-Windows, max_concurrency == 0 makes max_threads become 0, which underflows bg_thread_count = max_threads - 1 and can lead to a huge reserve()/spawn loop. This creates a crash / OOM hazard if a caller ever passes 0 (even accidentally). Consider clamping max_concurrency (or max_threads) to at least 1 (the calling thread).
        WorkCallbackContext<F> context{work, work_count};
        auto max_threads = std::min({work_count, max_concurrency, static_cast<size_t>(get_concurrency())});
        max_threads = std::min(max_threads, (SIZE_MAX - work_count) + 1u); // to avoid overflow in fetch_add
        auto bg_thread_count = max_threads - 1;
        std::vector<JThread> bg_threads;

@Crzyrndm JC (Crzyrndm) left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm unable to test this week but the change looks fairly similar to the original experiment otherwise

Comment thread src/vcpkg/binarycaching.cpp
@Crzyrndm

JC (Crzyrndm) commented Aug 4, 2026

Copy link
Copy Markdown

Testing results with 11 dependencies. Timing taken from the log line which looks like Restored 11 package(s) from AWS in 34 s. Use --debug to see more details. (34s)

Test run using ./vcpkg/vcpkg install --triplet=x64-windows --x-install-root=build/vcpkg_installed and deleting the build folder each run. The cache was an AWS bucket set by env

  • $Env:VCPKG_BINARY_SOURCES="clear;x-aws,s3://<bucket>/test1,readwrite"

Baseline (Tag 2026.07.29)

  • Cache miss: 13s
    • 13s
    • 13s
  • Cached: ~25s
    • 34s (possibly a cold path somewhere?)
    • 24s
    • 26s
    • 24s

This branch

Build is windows, using the Win-x64-Release preset and vcpkg_root is checked out to the same tag (2026.07.29) and the executable is just copied over the previous test

  • Cache miss:
    • 2.7s
    • 2.3s
  • Cached:
    • 6.7s
    • 7.1s
    • 6.5s

Summary

Compared to results in #1392 (comment)

  • the baseline appears to be slightly slower (13 instead of 9.5, 25 instead of 23) - partially due to 1 extra package, probably lots of different versions involved, etc.
  • This PR is ~50% slower than the original on my machine - likely due to different parallelism restriction (8 here, get_concurrency there). This is primarily for CI so a more limited value is sensible and is somewhat likely to reverse the change due to low core counts on CI machines

@BillyONeal

Copy link
Copy Markdown
Member Author

This PR is ~50% slower than the original on my machine - likely due to different parallelism restriction (8 here, get_concurrency there). This is primarily for CI so a more limited value is sensible and is somewhat likely to reverse the change due to low core counts on CI machines

Yeah I was really worried about introducing reliability problems by hammering remotes with like 100 concurrent queries on big build machines tripping DDOS prevention and that sort of thing.

@Crzyrndm

JC (Crzyrndm) commented Aug 5, 2026

Copy link
Copy Markdown

This PR is ~50% slower than the original on my machine - likely due to different parallelism restriction (8 here, get_concurrency there). This is primarily for CI so a more limited value is sensible and is somewhat likely to reverse the change due to low core counts on CI machines

Yeah I was really worried about introducing reliability problems by hammering remotes with like 100 concurrent queries on big build machines tripping DDOS prevention and that sort of thing.

I did see one example where there was a warning printed about rate limiting but it wasn't replicated and still succeeded. 8 seems like a safe-ish default to start with and the worst potential outcome currently is I believe a local build so not too worried about it for now.

  • May have been the first test run with parallel requests so potentially some kind of S3 scaling behaviour got triggered? If so, it would have been a cache miss test

@BillyONeal
Billy O'Neal (BillyONeal) merged commit ad905a5 into microsoft:main Aug 5, 2026
7 checks passed
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.

4 participants