Repository navigation
Snapshot uploads reserve only the memory their upload path uses - #2848
Merged
Max Smythe (maxsmythe) merged 1 commit intoOct 19, 2026
Merged
Max Smythe (maxsmythe) merged 1 commit into
Max Smythe (maxsmythe) merged 1 commit into
Conversation
plan() reserved max(streaming, ranged) for every file, because the ranged and streaming paths are alternatives. But plan() already knows which one runs: PutSparseFile splits a file whenever it has two or more parts. The streaming term grows with the budget and GOMAXPROCS, so on a 192-vCPU node with a 16Gi budget a 292 MiB snapshot reserved 2.6 GiB for a fallback that never runs, while its four parts hold 192 MiB. Only 5-6 uploads fit at once. - A file the GCS client splits into range parts reserves parts x sparseUploadMemory. - A streamed object whose sparse stream is bounded below uploadCompositeMin reserves the single request's buffers (PutObject's lookahead and the client's chunk buffer) instead of the composite part buffers. - Send sizes a small manifest's reservation the same way. Fixes agent-substrate#2847
Contributor
Author
| return err | ||
| } | ||
| memory := uploadBufferMemory(u.client) | ||
| if f, ok := content.(*os.File); ok { |
Collaborator
There was a problem hiding this comment.
🤖 should-fix 🟡 – The *os.File branch in Send is new behavior with no test; only plan is covered by TestUploaderReservesOnlyTheChosenPath. TestUploaderSlowGCSComposite already has the gated-transport harness: a sibling case that sends two small regular files at the minimum GCS budget and expects both to start would fail on main (320 MiB each) and pass here (128 MiB each).
| // PutSparseFile splits a file into parts whenever it has two or | ||
| // more (see errSparseTooSmall), so the streaming fallback never | ||
| // runs: reserve only what the parts hold. | ||
| return uploadPlan{workers: int(workers), parts: parts, memory: int64(parts) * sparseUploadMemory}, nil |
Collaborator
There was a problem hiding this comment.
🤖 nit 🟢 – This branch reserves only the part buffers on the assumption that PutSparseFile never returns errSparseTooSmall when parts >= 2. That holds today because both sides compute min(sparsePartCount(populated), maxParts), but the predicate is duplicated and nothing enforces it: if it ever fires, sendZstd falls through to streaming with the composite buffers and workers unreserved. Consider sharing the "splits" predicate between the two call sites, and having sendZstd return an error instead of falling back when plan.parts >= 2.
Collaborator
|
Aside: the commit message has "Fixes #2847", we ask in our docs that you put that only in the PR body. This is because github will close issues Across repos and forks if a merged commit refers to fixing an issue (e.g. when some people sync their forks, depending on their git workflow). Kubernetes actually has a robot BLOCK merging any commit with this problem. |
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
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.
Fixes #2847
Uploader.planreservedmax(streaming, ranged)for every file. A file that the GCS client splits into range parts never takes the streaming path, yet the streaming term grows with the budget andGOMAXPROCS. On a 192-vCPU node with a 16Gi budget, a 292 MiB snapshot reserved 2.6 GiB while its four parts hold 192 MiB, so only 5–6 uploads fit at once. Synchronous suspends spent 82% of their checkpoint time waiting for the budget; measurements are in #2847.This change reserves for the path an upload takes:
PutSparseFilewill split (two or more parts) reservesparts × sparseUploadMemory.uploadCompositeMinreserves that single request's buffers (theuploadCompositeMinlookahead plus the client'suploadChunkSizechunk buffer) plus its workers, instead of the composite part buffers.Sendsizes a small manifest's reservation the same way.S3 is unchanged: it has no range path and always buffers parts. The minimum budget (
384MiGCS,224MiS3) is unchanged too, because a streamed object larger than one request still needs the composite buffers.On the cluster described in #2847 (one run each, 64 SWE-bench actors next to synchronous probe suspends), probe suspend p95 went from 18.2 s to 3.7 s at 16Gi, and from 112–136 s to 42 s at the 512Mi default. Those runs used an earlier revision that didn't count the client's 64 MiB chunk buffer for single-request objects. This version reserves 64 MiB more per metadata file and manifest.
Validation:
go test -race ./pkg/objectstorage ./internal/objectstoreplugin/... ./cmd/objectstore-plugin -count=2passes.golangci-lint run ./pkg/objectstorage/...reports 0 issues;hack/verify/gofmt.shandhack/verify/boilerplate.shpass.BenchmarkConcurrentSnapshotUploadsis within run-to-run noise ofmain(3 runs each: 362–369 vs 371–373 files/s at the shared 512 MiB budget). Its in-memory destination doesn't take the GCS paths this changes.New
TestUploaderReservesOnlyTheChosenPath;TestUploaderPlansFromPopulatedBytesandTestUploaderReservesBothPipelinesare updated for the GCS reservations.Tests pass
Appropriate changes to documentation are included in the PR