feat(serialize): opt-in bounded-memory zstd encoder profile (#79) - #80
Merged
Merged
Conversation
The shared zstd encoder keeps one worker per GOMAXPROCS and each level-15 worker retains about 36 MB, so a 16-CPU host held roughly 560 MB after its first encode with no way to shrink it short of lowering the level. Add EncoderProfile with EncoderProfileDefault and EncoderProfileBoundedMemory. The bounded profile keeps a single worker with zstd's lower-memory buffers (about 42 MB at level 15 regardless of GOMAXPROCS) and produces byte-identical output at every level. Select it per call with WithEncodeProfile (EncodeOption) or on the config with WithEncoderProfile (ConfigOption) so WriteSidecar, EncodeToMetadata and S3 sidecars use it; the per-call option wins. The encoder cache is keyed by zstd mode and profile so the two profiles never share an instance. The profile is runtime-only and not serialized; the default profile and wire format v11 are unchanged. gin-index build gains -low-memory. BenchmarkEncoderProfile and make bench-encoder-profile report retained memory, allocations, time and compressed size at GOMAXPROCS 1, 4 and 16; results and the trade-off are documented in docs/encoder-profile-benchmarks.md and the README. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dh45hVm1GJoj9sTGDah84X
- Test byte-identical output on a multi-block (>128 KB) payload too, so the path where WithLowerEncoderMem changes buffer sizing is covered. - Keep forced GCs out of the cold benchmark's timed region and refresh the cold timings in docs/encoder-profile-benchmarks.md. - Add -low-memory to gin-index extract and experiment; extract passes the profile per call because a decoded index always carries the default. - Share one cache-key helper and one eviction helper across tests and benchmarks; serialize each benchmark payload once. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dh45hVm1GJoj9sTGDah84X
Close the encoder-profile review gaps on the bounded-memory zstd encoder feature (#79): - I1: WriteSidecar, EncodeToMetadata, RebuildWithIndex, and S3Client.WriteSidecar/WriteSidecarContext accept a trailing opts ...EncodeOption so a caller can force a profile even when idx.Config carries the default (post-decode) profile. - I2: add TestEncoderProfileBoundedUsesSingleWorker, asserting via reflection that the bounded profile configures a single zstd worker with lowMem=true. - I3/S10: correct EncoderProfileDefault/BoundedMemory doc comments to the measured numbers (~34 MB per additional worker, ~560 MB at GOMAXPROCS=16, ~42 MB bounded) and note the worker pool size is fixed at first encode. - S2: replace the tautological Header.Version check in TestEncoderProfileConfigReachesAllEncodePaths with a byte-equality check proving the serialized config payload does not carry the profile. - S5: wrap zstd encoder construction errors with level and profile context. - S6: use key.profile (not the shadowed profile param) as the single cache key identity source in sharedZstdEncoder. - S8: rewrite TestEncodeDecodeConcurrentMixedProfiles's doc comment and restore the decode-then-re-encode correctness check. - S9: clarify the zstdEncoderKey doc comment on who Closes cached encoders. - S11: improve the unknown-profile validation error message. Also updates the S3Client structural-compatibility test in boundary_observability_test.go for the new WriteSidecar/WriteSidecarContext variadic signature. Ref #79
…ts (#79) I4: gin-index experiment --low-memory without -o was a silent no-op; print "Warning: -low-memory has no effect without -o" to stderr instead. Extracts experimentGINConfig as a standalone, independently testable helper (mirroring buildGINConfig in main.go) instead of building the config inline in runExperiment. S1: add TestExperimentGINConfigLowMemorySelectsBoundedProfile (config-level profile assertion for the experiment command) and TestRunBuildLowMemoryProducesDecodableIndex (end-to-end runBuild -low-memory coverage for both sidecar and -embed modes). Amend the doc comments on TestRunExtractLowMemoryWritesSameBytes and TestRunExperimentLowMemoryWritesSameSidecarBytes to state they guard output bytes and exit code only. Ref #79
I3: align every prose memory figure with the measured benchmark tables -- "about 34 MB per additional worker", "about 560 MB at GOMAXPROCS=16", and "about 42 MB bounded" -- across README.md, CHANGELOG.md, docs/encoder-profile-benchmarks.md, and the -low-memory CLI flag help in cmd/gin-index/main.go (serialize.go's doc comments were already corrected). S4: clarify that Encode's zstd encoder cache is keyed by compression mode and profile (not the raw numeric level), and scope the "level 3 is larger" claim to the high-cardinality fixture, since the small fixture is smaller at level 3. S7: bump the klauspost/compress version noted in CLAUDE.md from v1.18.3 to v1.19.2 to match go.mod. Adds a footnote to docs/encoder-profile-benchmarks.md's retained-memory table naming the highcard fixture as the L3 column source and noting level-3 retention is sensitive to first-payload size. Ref #79
Vary the fixture status literal in TestRunBuildLowMemoryProducesDecodableIndex so it does not push an existing string past golangci-lint's goconst threshold, and document+suppress the unparam finding on sharedZstdEncoderCached: every current call site checks the bounded-memory profile, but the parameter mirrors evictSharedZstdEncoder's signature for a future default-profile assertion. Ref #79
- CHANGELOG: note trailing EncodeOption on WriteSidecar, EncodeToMetadata, RebuildWithIndex and S3Client sidecar helpers - README: per-call helper example; label level-3 retention by fixture - benchmarks doc: 'per additional worker'; reconcile level-3 bullet with tables - serialize_profile_test: strengthen per-call override test with a positive default-profile assertion and drop the unparam nolint; fix stale comments - main_test: correct which tests cover extract -low-memory wiring - serialize_concurrency_test: reference identifiers instead of line numbers
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.
Summary
Fixes #79.
Encodereuses one shared zstd encoder per compression mode, and each encoder keeps one worker perGOMAXPROCS. A level-15 worker holds about 36 MB of match tables, so a 16-CPU process retained about 560 MB after its first encode. Until now the only way to shrink that was a lower compression level.This PR adds an opt-in bounded-memory encoder profile. It keeps a single worker at the same compression level, and the output bytes are identical.
Status: Verified ✓ (quick-260925-b7m, hold-out expectations E1–E11 all pass)
Changes
Encoder profile
EncoderProfiletype:EncoderProfileDefault(today's behaviour) andEncoderProfileBoundedMemory.WithEncoderConcurrency(1)+WithLowerEncoderMem(true). The window size is unchanged, so output is byte-identical to the default profile at every level.(zstd mode, profile), so the two profiles never share an instance. It holds at most 4 × 2 entries.Selecting it
WithEncoderProfile(p)is aConfigOption. Every encode path that reads the index config uses it:Encode,WriteSidecar,EncodeToMetadata, S3 sidecars.WithEncodeProfile(p)is anEncodeOptionthat overrides the config per call, including an explicitEncoderProfileDefault.EncoderProfileDefault. Wire format staysv11.CLI
gin-index build,extractandexperimentgain-low-memory.extractpasses the profile per call, because a decoded index always carries the default.Benchmarks and docs
BenchmarkEncoderProfilecovers both profiles × levels 15 and 3 × small and high-cardinality fixtures × cold and repeated runs. It reports retained MB, allocations, time and compressed size.make bench-encoder-profileruns it atGOMAXPROCS1, 4 and 16. Results are indocs/encoder-profile-benchmarks.md.Unreleasedentry, and CLAUDE.md is updated.Retained heap after the first level-15 encode:
Review round
A high-effort self-review of the first commit found five issues; all are fixed in the second commit:
-low-memoryis added toextractandexperiment -o.Key files:
serialize.go,gin.go,cmd/gin-index/main.go,cmd/gin-index/experiment.go,serialize_profile_test.go,serialize_concurrency_test.go,benchmark_test.go,Makefile,README.md,CHANGELOG.md,docs/encoder-profile-benchmarks.mdRequirements Addressed
GOMAXPROCS1, 4 and 16, cold and repeated, at levels 15 and 3, on small and high-cardinality fixtures.Verification
go build ./...andgo vet ./...passmake lint: 0 issuesgo test -race -short ./...passes in all packages./... ./testdata/phase20): 1199 tests pass, 1 pre-existing skip (missingtestdata/test.parquet)make bench-encoder-profileran at GOMAXPROCS 1, 4 and 16make testexits non-zero in the authoring container only because that Go toolchain lacksgo tool covdatafor-coverprofile. No tests failed. CI's runner has the tool.Key Decisions
WithEncodeSignals/WithSignals.Review round 2 (local multi-agent review, 15 findings)
Summary
WriteSidecar(path, idx) EncodeToMetadata(idx) RebuildWithIndex(path, idx) S3Client.WriteSidecar(ctx?, bucket, key, idx) + ...opts ...EncodeOption // e.g. WithEncodeProfile; a decoded index always carries DefaultTestEncoderProfileBoundedUsesSingleWorkerreflects into the zstd encoder and asserts one worker and low-memory buffers. Before this, dropping both options passed every test.experiment -low-memorywithout-onow warns instead of doing nothing.Evidence
WithEncoderConcurrency(1)/WithLowerEncoderMem(true)fromnewZstdEncoderleft the suite green.After:
TestEncoderProfileBoundedUsesSingleWorkerfails on channel capacity or lowMem.go test -race -short . ./cmd/...passes.make lintreports 0 issues (Go 1.25.5 toolchain).Merge Danger
Door: two-way. Trailing variadic parameters are additive; wire format stays v11.
Blast Radius: small. Only callers that store these functions in a typed variable with the old signature would need a change.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Dh45hVm1GJoj9sTGDah84X
Generated by Claude Code