feat(build): single-pass layer compression behind WithCompressedLayerFile - #2479
Conversation
db21a3a to
692f677
Compare
692f677 to
b61ac3e
Compare
| defer outfile.Close() | ||
| lw := newLayerWriter(outfile) | ||
| var lw *layerWriter | ||
| if bc.o.CompressedLayerFile { |
There was a problem hiding this comment.
Would this all be a lot simpler if we got rid of the diffID cache, which is the reason this is two-pass in the first place? Should we instrument that cache and measure effectiveness first? If it's not really needed, maybe we get rid of it wholesale and make this the one-and-only way?
There was a problem hiding this comment.
Agreed on the causality — db97d1ee (#1735) added the laziness and the cache in one commit, and before it newLayerWriter was already tar → MultiWriter(diffid, gzw), which is what this reintroduces.
This PR is narrower on purpose: it's aimed purely at cutting peak memory on the build instances, where the double write is ~3 GiB of the 3.91 GiB scratch peak on a large image. It's opt-in and doesn't touch the cache, so the default path is unchanged.
Deleting the cache flips the default for every consumer, and as you say it wants the hit rate measured first. One data point for when we do: at a 100% hit rate (three identical 512 MiB layers) two-pass still lost, 1.476s vs 1.284s. The cache only skips the compression; the plain-tar write is unconditional and it's the larger cost. So it may well go your way, and if it does, this option comes out along with compress() and the cache.
Filed as #2485.
There was a problem hiding this comment.
Let's do that first, should be cheap to measure and guides us from the get-go.
There was a problem hiding this comment.
Happy to measure it.
The hit rate won't settle the whole question though. build-minirootfs writes the layer to a path the user names (<output.tar.gz>), and today that file is plain tar — same for anyone using WithTarball() or reading the path from BuildLayer(). Making single-pass mandatory changes that artifact for every consumer, and apko is a public module.
So there are two gates on flipping the default and the cache measurement only clears one. This PR is opt-in and touches neither, so I'd rather land it and make the default flip its own change.
There was a problem hiding this comment.
Correction to the 100% hit-rate number above: that benchmark was single-layer. A multi-layer A/B (five variants of one layered image, one process, hot cache) went the other way: two-pass ~1.8-1.9s vs single-pass ~2.2-2.3s per build, at 0.52 GiB vs 0.18 GiB peak scratch. So the cache does buy wall time when it hits, roughly 25%, and this option gives that up for the memory.
First production numbers from #2486 are on #2485: 47-79% hit rate during a full catalog rebuild, which is the cache's best case. Steady state pending. The PR body now states the trade with those numbers instead of the single-layer claim.
There was a problem hiding this comment.
+1 on if we could make this "the one way", though I think starting with a flag is reasonable so we can roll it out easier. I think the other callsites mentioned could be adapted so that this would be non-breaking.
There was a problem hiding this comment.
Agreed, and that is #2485. I audited the callers for it: everything except build-minirootfs reaches the layer through the v1.Layer interface and never reads the returned path, so a default flip is invisible to them. build-minirootfs is the exception, because it writes to a path the user names and that file is plain tar today. It either keeps writing plain tar explicitly or starts writing what the .tar.gz extension says, with a release note.
The cost for the rest is that Uncompressed() gunzips in this mode, which lands on consumers that extract a whole rootfs through it (melange among them). That is CPU, not breakage.
…File Tar layers are written twice today. ImageLayoutToLayer writes the layer as plain tar into a file named .tar.gz, and the first Digest/Size call triggers layer.compress(), which re-reads the whole file and writes a second, actually gzipped file beside it. Both are retained until the caller's cleanup, because the layer object backs publish. Peak scratch per layer is therefore roughly uncompressed + compressed size, which on a memory-backed temp dir is instance memory, and the largest builds OOM the instance. WithCompressedLayerFile() makes the layer writer gzip during the original write: tar bytes flow through the diffID hash into a pgzip writer whose output flows through the compressed-digest hash and a byte counter into the file. Only the compressed file ever exists, the descriptor is complete at finalize, and Uncompressed() gunzips on demand for the consumers that need tar bytes. Compressed output is byte-identical to the two-pass path (same pgzip level and 1 MiB block size), so blob digests do not change. The option is opt-in, so build-minirootfs and anyone consuming the returned path as plain tar are unaffected, and EROFS is untouched because it branches before any layer writer is constructed. Single-pass layers get their own v1.Layer implementation rather than a mode flag on the existing one, in the same shape as erofsLayer: every field set at construction, no locking, nothing lazy. splitLayers gives each concurrently open writer one pgzip worker to bound compressor memory, and failed builds now abort writer pipelines and remove partial layer files in both modes. Measured on a 3 GiB toolchain image: peak scratch 3.91 GiB -> 0.88 GiB (77%), with identical diffIDs and digests. Cold build ~5s -> ~3s. The warm path, where the two-pass side can skip compression on a compressionCache hit, is still faster single-pass (1.476s -> 1.284s across three identical 512 MiB layers) because it never writes the plain tar regardless. Co-authored-by: Claude <noreply@anthropic.com>
e6a989f to
2a74c3f
Compare
| diffid := sha256.New() | ||
| digest := sha256.New() | ||
| cw := &countingWriter{w: io.MultiWriter(digest, out)} | ||
|
|
||
| gzw := gzip.NewWriter(cw) | ||
| if err := gzw.SetConcurrency(1<<20, workers); err != nil { | ||
| return nil, fmt.Errorf("setting pgzip concurrency to %d: %w", workers, err) | ||
| } | ||
|
|
||
| w := tar.NewWriter(io.MultiWriter(diffid, gzw)) |
There was a problem hiding this comment.
Should this use the existing pooledGzipWriter()?
There was a problem hiding this comment.
Not as-is: pooledGzipWriter bakes in SetConcurrency(1<<20, pgzipThreads), and this path needs a variable count. Multi-layer builds hold every writer open across the walk and take one worker each, so compressor memory scales with layer count rather than layers times fan-out. A pooledGzipWriter(w, workers) could express that.
The hazard is the error path. Close sets z.closed = true and can return at its error check before reaching close(z.results). Reset only closes that channel when !z.closed, so a writer whose Close saw an output error returns to the pool with its output listener parked forever, still holding the old file. That goroutine isn't in pgzip's waitgroup, so init's wg.Wait() misses it too. Right now an ENOSPC leaks one goroutine; pooling would keep the entry.
Can add the parameter and pool it, but I'd want a fault-injection test for the poisoned writer first.
| // Abort shuts down the writer pipeline without producing a layer. Safe to | ||
| // call after finalize. | ||
| func (lw *layerWriter) Abort() { | ||
| if lw.abort != nil { | ||
| lw.abort() | ||
| } | ||
| } |
There was a problem hiding this comment.
Can finalize handle this rather than adding another method to the interface?
| // newCompressedLayerWriter wraps a file with a tar writer that gzips in the | ||
| // same pass, computing the diffID, compressed digest, and size as the bytes | ||
| // stream, so the plain tar never exists on disk. | ||
| func newCompressedLayerWriter(out *os.File, workers int) (*layerWriter, error) { |
There was a problem hiding this comment.
This shares a lot of machinery with newLayerWriter, can it be folded together? The function could decide which struct to create (compressedLayer vs layer) or possibly pass in io.ReadClosers to use for satisfying the interface (if collapsed into single layer struct)?
| defer outfile.Close() | ||
| lw := newLayerWriter(outfile) | ||
| var lw *layerWriter | ||
| if bc.o.CompressedLayerFile { |
There was a problem hiding this comment.
+1 on if we could make this "the one way", though I think starting with a flag is reasonable so we can roll it out easier. I think the other callsites mentioned could be adapted so that this would be non-breaking.
compressedLayer duplicated most of layer, and newCompressedLayerWriter duplicated most of newLayerWriter, to express what is really one difference: whether the tar is gzipped as it is written or later. Fold both pairs together. layer carries either mode, discriminated by singlePass(), and newLayerWriter takes the mode plus the pgzip worker count. finalize now always runs every close, even after one fails, so a live pgzip pipeline can no longer outlive a failed tar close; that makes it a complete teardown and removes the need for a separate Abort, which callers replace by discarding finalize's result. Digest and Size skip the compression cache for single-pass layers. They have nothing to look up, and counting the lookup would report a miss for work that never needed doing, skewing the cache metric from #2486. Co-authored-by: Claude <noreply@anthropic.com>
Every other option in `pkg/build` takes the value it sets.
`WithCompressedLayerFile` was a no-argument toggle, which forces any
caller driving it from configuration to branch:
```go
opts := []build.Option{
build.WithSBOMGenerators(spdx.New()),
build.WithAuthenticator(authn),
build.WithCache(cacheDir, false, apk.NewCache(false)),
}
if compressed {
opts = append(opts, build.WithCompressedLayerFile())
}
```
Taking the bool lets that collapse into the literal alongside its
siblings. It also means the caller's option slice keeps `len == cap`, so
it cannot hand `LockImageConfigurationWithPackages` spare capacity to
append into, which is the aliasing that function already clones around.
## Breaking change
`WithCompressedLayerFile()` becomes `WithCompressedLayerFile(enabled
bool)`. Existing calls become `WithCompressedLayerFile(true)`.
Landing it now because the blast radius is currently zero. The option
shipped in v1.4.0 a few hours ago, and there are no callers in this repo
outside the option's own definition and test, nor in melange or
terraform-provider-apko. That window closes as soon as something adopts
it.
## Tests
The existing round-trip gains an explicitly disabled case. It previously
covered the option present versus absent, so an implementation that
ignored its argument would have passed. Mutation-checked: hardcoding
`true` fails the new leg.
The doc comment also picks up the EROFS caveat, which held in v1.4.0 but
was only written down in #2479's description.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
3 participants
Problem
Tar layers are written twice.
ImageLayoutToLayerwrites the layer as plain tar into a file named.tar.gz(newLayerWriterstreams the diffID but never compresses, despite its comment), and the firstDigest()/Size()call triggerslayer.compress(), which re-reads the whole file and writes a second, actually-gzipped file beside it. Both files then live until the caller's cleanup, because the layer object backs publish.Peak scratch per layer is therefore roughly uncompressed + compressed size. For services that build in tmpfs-backed temp dirs (where scratch bytes are instance memory), large builds can hold roughly 1.4x the image's installed size in RAM per build, and the biggest builds OOM the instance.
Change
A new
WithCompressedLayerFile()option makes the layer writer gzip during the original write: tar bytes flow through the diffID hash into a pgzip writer whose output flows through the compressed-digest hash and a byte counter into the file. Only the compressed file ever exists (its.tar.gzname finally honest), the descriptor is complete at finalize, andUncompressed()gunzips on demand for the few consumers that need tar bytes (e.g. CPIO conversion). Peak scratch drops to roughly the compressed size.Details:
build-minirootfsand anyone consuming the returned path as plain tar are unaffected. Two error-path behaviors do change for both modes:ImageLayoutToLayernow reports the output file's close error instead of discarding it in adefer, and a failedsplitLayersdeletes its partial temp files rather than leaving them behind.Uncompressed()returns a decompressing reader in this mode, so CPIO/EFI conversion and any tar-reading SBOM path keep working. They pay a gunzip they didn't pay before, which is why the option is opt-in per caller rather than a default flip. The read path usescompress/gziprather than pgzip: decode is serial and feeds a streaming tar reader, so pgzip's read-ahead buffers and goroutine buy nothing here.--format erofsbranches before the layer writer is constructed and returns anerofsLayer, whoseUncompressed()is a plainos.Open, so the option is inert there. EROFS layers are uncompressed filesystem images by design, which means their peak scratch is already roughly the installed size and this change does not reduce it. A caller budgeting scratch per build should key offic.Formatrather than assuming the compressed-tar figure applies everywhere.compressionCachein this mode: laziness needs a materialized plain tar to defer compression against, which is exactly the file this option eliminates. Single-pass layers are a separatev1.Layerimplementation (compressedLayer) in the same shape as the existingerofsLayer, so every field is set at construction andDigest()/Size()need no lock, and single-pass layers never read or write that cache.splitLayersholds every layer writer open through the filesystem walk, so each gets one pgzip worker rather than themin(GOMAXPROCS, 8)fan-out a sequentialcompress()call gets. Single-layer builds are unaffected and use that same worker count as before, so their CPU profile is identical to the two-pass path. A build with a few large layers has less compression parallelism than before, partly offset by the compression now overlapping the I/O-bound walk. That case is not measured below.Abort, and a failedsplitLayersnow removes its partial temp files, so retried builds in the same directory no longer accumulate leftovers. One caveat worth stating rather than burying:Abortshuts the pgzip pipeline down viagzw.Close(), and pgzip'sClosereturns at its own error check before closing its results channel, so a writer that already recorded an output error (ENOSPC on the scratch dir, say) can still leave its listener goroutine parked. The teardown is reliable for input-side failures and best-effort for output-side ones.Why this isn't a two-line change
Flipping the writer to gzip is the easy part. The rest of the diff exists because compressing in the same pass changes three invariants the two-pass path relied on:
compress()deferred the compressed digest until someone asked, using the plain tar as the source it could re-read. Once that file doesn't exist, the digest and size have to be captured as the bytes stream, which means hashing and counting inline and sealing the descriptor at finalize.Uncompressed()has to gunzip. Consumers that want tar bytes previously got the file as-is. They now get a decompressing reader, which has to close both the gzip reader and the file underneath it.AbortandsplitLayersgained ordered teardown (abort every writer, then close and remove every file).Roughly 80 lines are that bookkeeping, and the rest is tests.
Measurements
Built with a local harness against real Wolfi images, sampling scratch usage every 50ms.
77% less peak scratch, with byte-identical diffIDs and digests on every image tested.
What bypassing the cache costs
The two-pass path can skip compression when
compressionCachealready holds the diffID. This mode never consults that cache, so every layer is compressed exactly once, on write. That is a CPU cost, and it is measurable: five variants of one layered image built sequentially in one process, where the cache is hot, ran ~1.8-1.9s per build two-pass against ~2.2-2.3s single-pass, at 0.52 GiB against 0.18 GiB peak scratch. At high hit rates the flag trades roughly 25% wall time for roughly 65% less scratch.How often the cache actually hits is #2485. Production ran 47-79% during a full catalog rebuild, which is the best case for a per-process diffID cache; the steady-state figure is still being collected. The option is opt-in because that trade is right for a memory-bound caller and wrong for a CPU-bound one.
What this means for callers that budget scratch
A caller deciding how many builds fit on a machine has to size scratch before the build runs, so the only number available is the image's installed size from package metadata. Against that baseline the same measurements read:
Both rows matter. The first is the failure this change addresses: a caller sizing scratch at installed size is under-provisioned by roughly a third, and on a memory-backed temp dir that gap is what turns a large build into an OOM.
The second is the part that is easy to miss. After the switch the same caller over-reserves by roughly 3x, which is safe but means the freed space does not become usable capacity on its own. Turning it into density requires the caller to scale its own estimate, and the compression ratio is image-dependent, so that multiplier should come from a distribution of real builds rather than any single measurement here.
Tests
WithCompressedLayerFile()driven end to end throughImageLayoutToLayer, asserting the option produces a gzip file and that the default does not. Mutation-checked in both directions: forcing the branch off fails it, and forcing it always-on fails it too.splitLayersto a mid-walk error in both modes and asserts the temp dir is emptied and no pgzip goroutines are left running (counted from a stack dump, so unrelated goroutines cannot flake it). Mutation-checked: removing the abort teardown fails it, and so does removing the partial-file cleanup.-raceand at-count=2, which pins their isolation against the process-global compression cache.One limit worth naming rather than leaving to be discovered:
gunzipReadCloser's two-handleCloseis exercised but not pinned. The round-trip proves the stream decodes, andCloseis asserted not to error, but a regression that released only one of the two handles would return nil and pass. There is no cheap way to observe a leaked file handle from outside the type.🤖 Generated with Claude Code