perf(storage): zero-copy upload path from mmap to GCS#2394
perf(storage): zero-copy upload path from mmap to GCS#2394ValentaTomas wants to merge 9 commits intomainfrom
Conversation
PR SummaryMedium Risk Overview Reviewed by Cursor Bugbot for commit 24f35a8. Bugbot is set up for automated code reviews on this repo. Configure here. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d84ad827b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ValentaTomas
force-pushed
the
perf/zero-copy-upload
branch
6 times, most recently
from
9a3ab85 to
f451748
Compare
Add StoreData(ctx, []byte) to the storage interface and wire up the template build upload path to use it. Memfile/rootfs uploads now go directly from mmap-backed diff data to HTTP, skipping the previous CachePath→StoreFile→ReadAt→alloc-per-chunk pipeline.
ValentaTomas
force-pushed
the
perf/zero-copy-upload
branch
from
c158470 to
00efb6a
Compare
The dataStorer type assertion in uploadDiff always failed because OpenSeekable returns a cachedSeekable or peerSeekable wrapper, not the inner gcpObject that implements StoreData. This made the zero-copy upload path unreachable, always falling back to StoreFile. Add StoreData forwarding to cachedSeekable and peerSeekable so the type assertion succeeds through the wrapper chain. Also release the mmap lock (from diff.Data) before falling back to StoreFile, since that path reads from disk and doesn't use the mmap.
| } | ||
|
|
||
| // StoreData forwards to the inner storage provider for zero-copy upload from mmap'd data. | ||
| func (c *cachedSeekable) StoreData(ctx context.Context, data []byte) error { |
There was a problem hiding this comment.
if we ever want to start saving data to outer layer (NFS cache) this path is missing that
There was a problem hiding this comment.
I think the experiment with this didn't much work out as the NFS cache can't scale in these situations, so we will probably solve that differently.
- Extract tryZeroCopyUpload helper so defer release() works safely without risking deadlock on the StoreFile fallback path - Fix zero-size diff: when Data() returns nil, fall through to StoreFile instead of silently skipping the upload - Unify dataStorer interface as storage.DataStorer in one place - Fix gcsOperationAttr labels in StoreData (WriteFromData instead of WriteFromFileSystem) - Include concrete type in assertion error messages for debugging - Nit: remove redundant []byte() cast on mmap in cache.Data()
- Skip upload entirely when diff is *NoDiff (zero-size file), fixing the regression where NoDiff.CachePath() returned NoDiffError on the StoreFile fallback path - Add StoreDataViaFile fallback in cachedSeekable and peerSeekable so StoreData gracefully degrades via a temp file + StoreFile when the inner storage doesn't implement DataStorer, instead of returning an error
ValentaTomas
force-pushed
the
perf/zero-copy-upload
branch
2 times, most recently
from
24d42d2 to
f7a9540
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f7a9540. Configure here.
ValentaTomas
force-pushed
the
perf/zero-copy-upload
branch
4 times, most recently
from
4eeaa48 to
7d1cdf9
Compare
…ap upload Replace StoreFile(ctx, path) with Store(ctx, []byte) across all storage backends. The mmap-backed diff data now flows directly to GCS multipart upload as sub-slices, avoiding the previous CachePath→file open→ReadAt→ alloc-per-chunk pipeline. This merges the three-function multipart upload path (UploadFileInParallel + uploadParts + UploadData) into a single Upload(ctx, []byte, concurrency) method, and eliminates the separate DataStorer interface and StoreData methods on all wrappers.
ValentaTomas
force-pushed
the
perf/zero-copy-upload
branch
from
7d1cdf9 to
a43759c
Compare
The orchestrator smoke test runs Firecracker VMs that need network access. Without an explicit timeout, Go's default 10m applies, which is too short when the VM has network delays.
3 participants
Upload memfile/rootfs directly from mmap to GCS, skipping CachePath→StoreFile→ReadAt→alloc-per-chunk. Stacked on #2393.