Skip to content

Conversation

sbhavani
Copy link
Contributor

@sbhavani sbhavani commented Oct 4, 2025

Adds maxConcurrentDownloads parameter to ImageStore.pull().

Performance improvement: ~1.2-1.3x faster pulls for multi-layer images with higher concurrency.

@dcantah
Copy link
Member

dcantah commented Oct 6, 2025

Performance improvement: ~3-5x faster pulls for multi-layer images with higher concurrency.

Can you provide some details on the images you tried and actual time deltas?

@sbhavani
Copy link
Contributor Author

sbhavani commented Oct 6, 2025

@dcantah I haven't tested many images but here's a real world example with more modest speedups:

Image: nvcr.io/nvidia/cuda:12.6.2-base-ubuntu24.04
Size: 866.9 MB

Concurrency Time (real) Speedup vs Sequential
1 (sequential) 21.674s 1.0x baseline
3 (default) 17.761s 1.22x faster (18% improvement)
6 16.514s 1.31x faster (24% improvement)

Time Savings:

  • Concurrency 3: Saves ~3.9 seconds (18% faster)
  • Concurrency 6: Saves ~5.2 seconds (24% faster)

System info:
Model Name: MacBook Air
Chip: Apple M3
Memory: 16 GB

Network info:

  • Download: 297.5 Mbps
  • Upload: 5.8 Mbps

@sbhavani
Copy link
Contributor Author

sbhavani commented Oct 7, 2025

Sorry I overlooked this before but the default was set at 8 which is pretty reasonable for my current connection over VPN (se below).

  • Concurrency 8: 10.267s
  • Concurrency 12: 9.239s (10% improvement)

But there may be cases where users have slower connection and need lower concurrency or faster connection and can handle higher concurrency values. Also helps to align with Docker which sets a default of 3.

@dcantah
Copy link
Member

dcantah commented Oct 8, 2025

Can you run make fmt and repush?

@dcantah dcantah requested a review from adityaramani October 8, 2025 23:48
@dcantah
Copy link
Member

dcantah commented Oct 8, 2025

cc @adityaramani, I'd like your eyes on this. This seems decent to support, it's a nice little toggle.

Copy link
Contributor

@adityaramani adityaramani left a comment

Choose a reason for hiding this comment

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

Mostly a nit: But should we keep the default as 8? Same as it used to be before?

@dcantah
Copy link
Member

dcantah commented Oct 9, 2025

@adityaramani Was the 8 chosen originally based on some heuristic we had? Don't have the history

@dcantah
Copy link
Member

dcantah commented Oct 10, 2025

Oop, @sbhavani didn't notice this but we require signed commits (gpg, ssh etc.). Can you sign with one of those methods and repush? I'd just squash down to one commit and sign that.

@sbhavani sbhavani force-pushed the feature/parallel-layer-downloads branch from c2f0bef to 49d20f3 Compare October 10, 2025 01:34
@sbhavani sbhavani requested a review from dcantah October 10, 2025 01:59
@dcantah
Copy link
Member

dcantah commented Oct 13, 2025

@sbhavani It seems the squashed commit still isn't signed :( I don't mean a "signed off by" line if it wasn't clear, but an actual signature via ssh/gpg etc.

Add `maxConcurrentDownloads` parameter to `ImageStore.pull()` to allow
callers to control download concurrency. Previously, the concurrency was
hardcoded to 8 concurrent downloads. This change makes it configurable
with a default of 3 to better respect network constraints and registry
rate limits.

Changes:
- Add maxConcurrentDownloads parameter to ImageStore.pull() (default: 3)
- Add maxConcurrentDownloads parameter to ImportOperation
- Update fetchAll() to use configurable concurrency

Signed-off-by: Santosh Bhavani <[email protected]>
@sbhavani sbhavani force-pushed the feature/parallel-layer-downloads branch from 49d20f3 to 1a0c85b Compare October 14, 2025 03:53
@dcantah dcantah merged commit 87e8fd1 into apple:main Oct 14, 2025
1 check 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.

3 participants