Repository navigation
Limit memory used by concurrent snapshot uploads - #2708
Merged
Benjamin Elder (BenTheElder) merged 2 commits intoOct 16, 2026
Merged
Benjamin Elder (BenTheElder) merged 2 commits into
Benjamin Elder (BenTheElder) merged 2 commits into
Conversation
Reserve compression and backend upload buffers from a shared node budget before starting each file. Cap worker parallelism to fit, cancel queued work promptly, and retain reservations through worker cleanup. Upload manifests after their data files succeed. Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
Eitan Yarmush (EItanya)
requested review from
Benjamin Elder (BenTheElder),
ILLUM1N0X,
Anish Ramasekar (aramase) and
Dmitry Berkovich (dberkov)
and removed request for
Benjamin Elder (BenTheElder) and
Anish Ramasekar (aramase)
October 15, 2026 19:03
Comment on lines
+106
to
+107
| workers = min(workers, info.Size()/parZstdChunk+1) | ||
| parts = min(sparsePartCount(info.Size()), int(u.budget.capacity/sparseUploadMemory)) |
Collaborator
There was a problem hiding this comment.
🤖 should-fix 🟡 – info.Size() is the logical file size, so a sparse VM memory file (such as a 2 GiB file with < 128 MiB populated) reserves the maximum worker and range-part counts even when PutSparseFile rejects it with errSparseTooSmall and writeSparseZstd only compresses the populated extents. Size workers and parts from the populated extent byte count (and pass workers through to newParZstd in pkg/objectstorage/sparsezstd.go:83) so sparse files with small resident sets do not over-reserve the budget.
Collaborator
There was a problem hiding this comment.
We've hit this a few times.
Collaborator
Author
There was a problem hiding this comment.
Good point. The reservation now uses populated extents, so holes no longer inflate the worker and part counts.
|
|
||
| // Send uploads uncompressed content, using the same budget as SendFile. This is | ||
| // used for snapshot manifests; the reader is streamed without retaining its body. | ||
| func (u *Uploader) Send(ctx context.Context, url string, content io.Reader) error { |
Collaborator
There was a problem hiding this comment.
🤖 nit 🟢 – Replacing SendBytesToGCS with Uploader.Send in uploadManifest dropped the "sendBytesToGCS" trace span that SendFile kept on line 121, and left SendBytesToGCS (pkg/objectstorage/objects.go:134) and SendFileToGCSWithZstd (pkg/objectstorage/objects.go:175) with no callers in the repo. Start a trace span in Send and remove the two unused functions in pkg/objectstorage/objects.go.
Collaborator
Author
There was a problem hiding this comment.
Restored the span in Send and removed the two unused helpers.
Count populated bytes when planning compression workers and range parts so holes do not consume upload capacity. Preserve the file offset during planning and cover concurrent small sparse uploads. Restore the manifest upload trace span and remove the superseded upload helpers. Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
Benjamin Elder (BenTheElder)
approved these changes
Oct 16, 2026
2 tasks done
Benjamin Elder (BenTheElder)
pushed a commit
to BenTheElder/substrate
that referenced
this pull request
Oct 20, 2026
…oad buffers (agent-substrate#2835) Part of agent-substrate#2553. Related to agent-substrate#2502 and agent-substrate#2708. Large GCS snapshots (`PutSparseFile` → `writeSparsePart`) previously compressed each 64 MiB part as one unsized streaming zstd frame with freshly allocated buffers. `agent-substrate#2502`'s `parZstdReader` only parallelizes frames whose size is declared or bounded at `≤ 16 MiB`; a part written as one streaming frame of 64 MiB or more is neither, so restore goes serial from the first part. Meanwhile, switching parts to the worker-pool `parZstd` with one worker uses ~77 MiB peak per part (2.4× over `agent-substrate#2708`'s 32 MiB `serialWorkerMemory` budget). This PR: - Adds an inline mode to `parZstd` for `workers == 1`: no goroutines; each `≤ 8 MiB` chunk is compressed on the caller's goroutine into a frame that declares its size. `writeSparsePart` uses `newParZstd(dst, 1)`, and small files get it too. - Pools the 8 MiB chunk, 8.5 MiB frame, and 1 MiB sparse copy buffers, and removes unused `newSerialZstd`. - Makes `Write` and a second `Close` after `Close` return `os.ErrClosed`. - Adds `BenchmarkWriteSparsePart`, `BenchmarkDecodeSparse`, and format tests, including one that pins the output byte for byte to per-chunk `EncodeAll`. ### Results (`-count=10`, 512 MiB populated / 2 GiB sparse) | Benchmark | `main` | `agent-substrate#2502` as-is | **this PR** | |---|---|---|---| | **Write 64 MiB part:** time / peak heap (32 MiB budget) | 148 ms / 9.8 MiB | 133 ms / **76.9 MiB** ❌ | 144 ms / **26.4 MiB** ✅ | | **Write 64 MiB part:** frames with content size | 0 / 1 | 8 / 9 | **9 / 9** | | **Read 512 MiB:** time (this PR + `agent-substrate#2502`: 254 ms, 3.8×) | 971 ms | 285 ms (3.4×) | **552 ms (1.76×)** | | **`ConcurrentSnapshotUploads` (`shared_512MiB`):** `B/op` / peak heap¹ | 1,326 / 272 MiB | 1,326 / 286 MiB | **70 / 196 MiB** | - **Serial restore is 1.76× faster even without `agent-substrate#2502`:** sized frames decode straight into a buffer of the declared size (`runtime.memmove` drops from 40% to 9% of decode CPU). - **Stress case (`all_80_admitted`)¹:** batch time roughly halves (`57.6 → ~29 ms`) and peak heap drops about a third (`1,994 → 1,330 MiB`). - **Cluster A/B (`sweperf`, 150u on `c3-highcpu-192-metal`, with `agent-substrate#2502`):** microVM `restore_golden_download` −31% at p50 (`1,095 → 752 ms`) and −63% at p95 (`3,419 → 1,255 ms`); `restore_p50_s` −29% (`945 → 669 ms`). ¹ This PR's upload numbers are from a later session on the same machine than the `main` and `agent-substrate#2502` columns, so treat the deltas as approximate. ### Compatibility & follow-up - Snapshot format is unchanged; old and new snapshots decode byte-exact on both readers. - `agent-substrate#2502`'s `newParZstd(dst, 1)` in `writeSparsePart` lands as-is; only `TestReadZstdFrameBoundsUnsizedSmallFrames` needs to build its unsized frame with `zstd.NewWriter(nil).EncodeAll`. - Follow-up (now that `agent-substrate#2848` is merged): 1-worker uploads could reserve `serialWorkerMemory` instead of `parallelWorkerMemory`. - [x] Tests pass - [x] Appropriate changes to documentation are included in the PR (none needed)
Da Huang (git286)
pushed a commit
to git286/substrate
that referenced
this pull request
Oct 20, 2026
…nt-substrate#2945) ## Summary Follow-up to agent-substrate#2848 (and agent-substrate#2847). While agent-substrate#2848 fixed upload memory accounting for multi-part ranged sparse files, small single-request files (`< 64 MiB`) still allocated and reserved worst-case `64 MiB` buffers at three layers (`64 MiB` GCS lookahead + `64 MiB` GCS chunk buffer + `64 MiB` zstd worker = `192 MiB` per file, and `128 MiB` for `manifest.json`). Under the default `--upload-memory-budget=512Mi`, a small gVisor snapshot (~7 files + `manifest.json`, ~17 MB total) still required `> 1.2 GiB` of budget reservations, forcing concurrent `SuspendActor` uploads to queue behind the semaphore (2–3 files at a time). ### Changes - **Proportional GCS single-request buffers**: When the upload stream has a known size bound below `64 MiB`, size both the `PutObject` lookahead buffer and the single-request `Writer.ChunkSize` to the actual object size (rounded up to `256 KiB` multiples, `512 KiB` minimum total instead of `128 MiB`). - **Right-sized zstd compression chunks**: For sparse files with `< 8 MiB` of populated data, scale the per-frame compression chunk and zstd window down to a power of two in `[1 MiB, 8 MiB]`, reducing per-worker memory from `64 MiB` down to as low as `8 MiB`. - **Budget accounting**: Update `Uploader` planning to reserve these proportional buffer sizes (`8.5 MiB` minimum for small sparse files instead of `192 MiB`), allowing all files in a snapshot burst to upload concurrently within the default `512Mi` budget. Large files (`>= 64 MiB`) and ranged sparse uploads (`parts >= 2`) are unchanged. ## Test Plan ### 1. GKE Cluster Benchmark (`glutton_baseline_10_users`, 10 gVisor workers, default `512Mi` budget) | Metric (`grpc_SuspendActor`) | Oct 15 Baseline (pre-agent-substrate#2708) | `main` (`90de075f`, with agent-substrate#2848) | This PR | Change vs `main` | | :--- | :---: | :---: | :---: | :---: | | **p50** | 400 ms | 850 ms | **390 ms** | **-54.1%** | | **p75** | — | 1,000 ms | **420 ms** | **-58.0%** | | **p90** | 460 ms | 1,400 ms | **450 ms** | **-67.9%** | | **p95** | — | 1,600 ms | **520 ms** | **-67.5%** | | **p99** | 610 ms | 1,700 ms | **640 ms** | **-62.4%** | | **Average** | — | 905.4 ms | **414.3 ms** | **-54.2%** | | **Throughput** | — | 4.39 req/s | **5.73 req/s** | **+30.5%** | ### 2. Microbenchmarks (`pkg/objectstorage/upload_bench_test.go`) | Benchmark (`512 MiB` budget, 50 ms RTT) | `main` (agent-substrate#2848) | This PR | Speedup / Memory | | :--- | :---: | :---: | :---: | | `BenchmarkConcurrentSnapshotUploads/gcs_shared_512MiB` (80 small files) | 2,085 ms / 5,375 MiB | **73.6 ms / 83.2 MiB** | **28.4x faster, -98.5% B/op** | | `BenchmarkLargeSparseUpload/4_actors_gvisor_7files_each` (4× [256 MiB ranged + 6× 64 KiB]) | 905.5 ms / 1,613 MiB | **358.4 ms / 138 MiB** | **2.5x faster, -91.4% B/op** | | `BenchmarkLargeSparseUpload/1_actor_256MiB_ranged` (large file only) | 115.6 ms | **114.6 ms** | Unchanged | Part of agent-substrate#2553
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When several agents idle-suspend at the same time, their snapshot uploads allocate compression and storage buffers independently. Ten simultaneous snapshots with eight files each can therefore produce a large memory spike in the objectstore plugin.
This adds a shared upload memory budget across requests, with a 512 MiB default configurable through
--upload-memory-budget. Uploads reserve capacity for compression and S3/GCS buffers before starting, reduce compression concurrency when needed, and wait when the budget is exhausted. Cancellation and failures release capacity after workers finish. Manifests included with data files are uploaded only after those files succeed.The budget limits reserved upload buffers and compression state; it is not a process RSS limit. Queuing trades burst throughput for lower peak memory. The plugin README documents configuration, minimum budgets, and accounting limits.
Part of #2553. This addresses simultaneous snapshot uploads and is independent of the small-file allocation optimization in #2633.
Validation:
hack/verify-all.shpassed.go test -race -p 2 ./... -timeout=5m) hit the existingTestServerTLSConfigReloadsCACertsWithoutRestartflake in the benchmarking fake-atelet, reproduced on unchanged upstream (3/5 attempts). The production atelet package passed.NO_COLOR=1and passed twice with that variable unset.One synthetic run with 80 small files and a simulated 20 ms destination stall measured the following tradeoff:
These are compression backpressure measurements, not cloud throughput or process RSS measurements.