Repository navigation
Stop small snapshot files reserving full-size compression buffers - #2633
Conversation
… pool filled eagerly and writeSparseZstd always forced a full 8 MiB chunk, so a 36-byte file wasted 16 MiB
Eitan Yarmush (EItanya)
left a comment
Eitan Yarmush (EItanya)
left a comment
There was a problem hiding this comment.
🤖 assisted review, comments make sense to me though
| <-p.quota | ||
| created++ | ||
| } | ||
| if want := 2 * parZstdQueue; created != want { |
There was a problem hiding this comment.
With GOMAXPROCS=1, newParZstd clamps the pool to one worker, so this fails with pool allowance = 2 buffers, want 4. Can we derive the expected cap from the effective worker count, or request one worker here?
| // One buffer is already held in p.buf; drain the rest of the allowance. | ||
| created := 1 | ||
| for len(p.quota) > 0 { | ||
| <-p.quota | ||
| created++ | ||
| } |
There was a problem hiding this comment.
Can we exercise getBuf() here instead of just counting quota tokens? This test still passes if getBuf() allocates another buffer after the quota is exhausted, so it doesn't catch the regression it's intended to guard against. Acquire buffers up to the cap, check that the next request blocks, then return a buffer and verify that the request resumes and reuses it.
| } | ||
|
|
||
| // Round-trip at the sizes where the new chunk sizing changes behavior. | ||
| func TestSparseZstdRoundTripAtChunkBoundaries(t *testing.T) { |
There was a problem hiding this comment.
Optional: can we add these boundary cases to the existing TestSparseZstdRoundTrip table? It already handles the setup, decoding, and byte comparison, so keeping the cases together would avoid duplicating those assertions. We can also use strconv.FormatInt(n, 10) for the case names and drop the custom name formatter.
… counting tokens before instead of calling getBuf(), so it would've passed even if getBuf() went over the limit. also made the limit match the worker count since GOMAXPROCS clamps it, and moved the boundary sizes into the existing round-trip table.
|
Thanks, all three points were super fair! The second one especially—you're totally right that it wasn't testing what it claimed. It was only counting tokens, so getBuf() could have gone right past an empty quota and it still would've passed. I rewrote it to grab buffers up to the limit, make sure the next request blocks, and then return one to check that the waiter wakes up with that exact same buffer. I even hacked getBuf() to temporarily ignore the quota just to verify the test actually catches it (it does now, and the old one didn't). For GOMAXPROCS=1, the allowance now pulls from p.workers instead of the manual number I passed in, since newParZstd clamps down to GOMAXPROCS. Verified it by running GOMAXPROCS=1 go test. And I moved the boundary sizes into the TestSparseZstdRoundTrip table like you suggested, which let me delete the extra duplicate test and the custom name helper. |
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 agent-substrate#2553. This addresses simultaneous snapshot uploads and is independent of the small-file allocation optimization in agent-substrate#2633. Validation: - `hack/verify-all.sh` passed. - Object storage, plugin, and affected atelet snapshot/upload tests passed twice with the race detector. Coverage includes concurrent snapshots, cancellation, failures, manifest ordering, and S3/GCS buffering through mocked endpoints. - The full race suite (`go test -race -p 2 ./... -timeout=5m`) hit the existing `TestServerTLSConfigReloadsCACertsWithoutRestart` flake in the benchmarking fake-atelet, reproduced on unchanged upstream (3/5 attempts). The production atelet package passed. - Color-formatting tests failed with this shell's `NO_COLOR=1` and passed twice with that variable unset. - No live cluster or cloud throughput tests were run. One synthetic run with 80 small files and a simulated 20 ms destination stall measured the following tradeoff: | Admission | Sampled peak heap growth | Batch completion | | --- | ---: | ---: | | Shared 512 MiB budget | 279.7 MiB | 225.146 ms | | All 80 uploads admitted | 1,305 MiB | 44.592 ms | These are compression backpressure measurements, not cloud throughput or process RSS measurements. - [ ] Tests pass (affected tests pass; existing TLS flake noted above) - [x] Appropriate changes to documentation are included in the PR --------- Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
|
Abhishek Sharma (@abhisheksharma2411) this PR and #2835 both change buffer allocation in What each one does:
They seem complementary: #2633 keeps peak memory low for small files, and #2835 cuts allocation churn for full-size chunks. A combined version could use Options:
I'm happy with whichever works best for you and the reviewers. Once we pick, I can rerun both PRs' benchmarks on the combined version. |
|
Good catch on that before we ran into a merge conflict! I'd vote for Option 1: let's land this smaller fix first, then rebase #2835 on top of it. Here is why: This change is small, already reviewed by Eitanya, all his feedback is addressed, and the checks are green. PR #2835 is massive (+754/-23) and touches the on-disk sparse-part framing, so it's a lot heavier to use as a gate for a quick memory fix. The good news is that your putChunk logic already handles the sizing cleanly: it only returns a buffer to the pool when cap(b) == parZstdChunk. That means smaller files get their own sized buffers and never touch the pool, while full 8 MiB chunks do. It's the exact split you had in mind. Totally fine to go the other way if reviewers prefer it, though. And yes, definitely let's rerun both benchmarks once we decide! |
|
#2945 merged yesterday was there some overlap with that? |
5 participants
This fixes the first half of #2553, where super small files were eating way more compression memory than they had any right to.
In
pkg/objectstorage/parzstd.go, two things were stacking up:workers × parZstdQueuebuffers of 8 MiB each were being allocated right off the bat before a single byte got compressed.writeSparseZstdwas always demanding at least one worker at the full 8 MiB chunk size.So, a tiny 36-byte metadata file was casually reserving 16 MiB of memory just to exist.
The fix:
chunkFor(size)picks a frame size that actually matches the input size, with a 64 KiB floor and a little extra wiggle room for the headers.The Receipts (Numbers from tests):
The on-disk format stays identical. Round trips are fully tested across all the weird edge-case sizes where the new chunk sizing kicks in (0, 36 bytes, right around 64 KiB, right around 8 MiB, and multi-chunk). There's also a safety check to make sure the pool still refuses to grow past its cap — since switching to lazy filling could easily have turned a bounded pool into an unbounded monster.
I left out the shared memory budget across concurrent snapshots for now, since that feels like a separate design conversation. Totally down to follow up on that later though!
(P.S. Used AI tooling like Claude Code to help draft this change.)
Part of #2553