Skip to content

casblob: reuse the compressed-output buffer to cut upload memory churn#907

Open
mjonss wants to merge 2 commits into
buchgr:masterfrom
mjonss:reuse-compressed-chunk-buffer
Open

casblob: reuse the compressed-output buffer to cut upload memory churn#907
mjonss wants to merge 2 commits into
buchgr:masterfrom
mjonss:reuse-compressed-chunk-buffer

Conversation

@mjonss

@mjonss mjonss commented Jul 13, 2026

Copy link
Copy Markdown

Problem

Under a burst of concurrent uploads, bazel-remote can suffer a large transient heap overshoot (and OOM on memory-constrained hosts) despite small steady-state memory. alloc_space heap profiling attributes the bulk to zstd.(*Encoder).encodeAll, reached via casblob.WriteAndClose.

Root cause

The zstd write path compresses each blob in 1 MiB chunks and called EncodeAll(in, nil) per chunk, allocating a fresh output slice for every chunk of every upload. The chunk size is exactly 1<<20, and klauspost's EncodeAll only pre-allocates an output buffer when len(src) < 1<<20 (strictly less-than). A full 1 MiB chunk therefore starts from a nil dst and grows it by repeated append-doubling — several MiB of transient garbage per chunk.

Fix

Thread a reusable dst through zstdimpl.EncodeAll (both the pure-Go and cgo backends already accept one) and have casblob.WriteAndClose allocate a single output buffer, sized to compressBound(chunkSize), reused across all chunks of the blob. Sizing to the ZSTD_compressBound worst case guarantees EncodeAll never has to grow (and reallocate) the buffer, even for incompressible chunks whose compressed output is slightly larger than the input.

This complements #775 (which pooled the input chunk buffer) by removing the per-chunk output allocation, and is complementary to #475 (which tunes the encoder's internal memory rather than the output buffer).

Numbers

BenchmarkWriteAndCloseZstd (16 MiB blob, incompressible data), added in this PR:

B/op allocs/op
before ~82.8 MB 215
after ~1.1 MB 20

All existing tests pass.

Under a burst of concurrent uploads, bazel-remote could be OOM-killed by a
transient Go heap overshoot even though idle/steady-state memory is small.
Heap profiling (alloc_space) pointed at zstd.(*Encoder).encodeAll: the write
path compresses each blob in 1 MiB chunks and called EncodeAll(in, nil) per
chunk, so a fresh output slice was allocated for every chunk of every upload.

The chunk size is exactly 1<<20, and klauspost's EncodeAll only pre-allocates
an output buffer when len(src) < 1<<20 (strictly less-than). A full chunk
therefore starts from a nil dst and grows it by repeated append-doubling, so
each 1 MiB chunk churned several MiB of transient garbage. With no memory
backpressure on concurrent Puts, the allocation rate outran the GC and the
process was killed.

Thread a reusable dst through zstdimpl.EncodeAll (both the pure-Go and cgo
backends already accept one) and have casblob.WriteAndClose allocate a single
output buffer, sized to compressBound(chunkSize), reused across all chunks of
the blob. Sizing to the ZSTD_compressBound worst case ensures EncodeAll never
has to grow (and reallocate) the buffer, even for incompressible chunks whose
output is slightly larger than the input.

A microbenchmark of the write path (16 MiB blob, incompressible data) shows
per-upload allocations drop from ~79 MB/op to ~1.1 MB/op.
@mjonss mjonss closed this Jul 13, 2026
@mjonss

mjonss commented Jul 13, 2026

Copy link
Copy Markdown
Author

I found this while setting up bazel-remote on a small node with limited memory, and it had OOM issues, so after investigating I came up with this PR.

@mjonss mjonss reopened this Jul 13, 2026
@mjonss

mjonss commented Jul 13, 2026

Copy link
Copy Markdown
Author

I decided to make a smaller change, instead of using a sync.Pool since the benefit was very good to start with for my use case. If you find it better to also include a sync.Pool also for the compressedChunkBuffer as it already is for uncompressedChunk, I'm happy to add that too.

@mostynb mostynb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks- this looks pretty good, I'm a little worried that the go zstd library might not provide a guarantee on the upper limit of the size of the compressed output. Maybe we should ask Klaus?

Comment thread cache/disk/casblob/casblob_test.go Outdated
Comment thread cache/disk/casblob/casblob.go Outdated
Comment thread cache/disk/casblob/casblob.go Outdated
Comment thread cache/disk/zstdimpl/gozstd.go Outdated
Comment thread cache/disk/zstdimpl/cgozstd.go Outdated
Comment thread cache/disk/zstdimpl/zstdimpl.go Outdated
- zstdimpl.EncodeAll: rename params to (src, dst), matching the order
  github.com/klauspost/compress/zstd uses.
- WriteAndClose: drop the compressBound() helper; size the reused output
  buffer inline to ZSTD_COMPRESSBOUND (srcSize + srcSize>>8 for a >= 128 KiB
  input) and document the bound for both backends.
- GetZstdReadCloser: pass a nil dst (restores the pre-change allocate-fresh
  behaviour on this one-shot, rarely-hit recompress path).
- benchmark: fix a stale comment reference (link to the PR instead of a
  non-existent local file) and trim the comments.
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.

2 participants