Skip to content

feat(serialize): opt-in uncached bounded-memory encoder profile (#83) - #86

Merged
oss-amikos merged 10 commits into
mainfrom
issue-83-bounded-memory-no-retention
Oct 6, 2026
Merged

oss-amikos merged 10 commits into
mainfrom
issue-83-bounded-memory-no-retention

Conversation

@oss-amikos

Copy link
Copy Markdown
Contributor

Summary

Fixes #83.

EncoderProfileBoundedMemory keeps its one-worker encoder in the shared encoder cache until the process stops. At level 15 that is 42 MB of live heap. A batch job that encodes one index per batch pays for it in every later step.

This PR adds a third profile, EncoderProfileBoundedMemoryUncached. The library builds the same one-worker encoder for one call and holds no pointer to it after the call.

 encodeWithLevel(idx, level, profile)
   profile.validate()
   body = serialize index
   if level == CompressionNone: return "GINu" + body
-  encoder = sharedZstdEncoder(level, profile)      # cache lookup, entry lives forever
+  if profile == EncoderProfileBoundedMemoryUncached
+    encoder = newZstdEncoder(mode(level), profile) # 1 worker, low-memory buffers, no cache entry
+  else
+    encoder = sharedZstdEncoder(level, profile)    # rejects the uncached profile with an error
   return "GINc" + encoder.EncodeAll(body)

Callers select it with the options that exist today:

gin.NewConfig(gin.WithEncoderProfile(gin.EncoderProfileBoundedMemoryUncached))                      // config
gin.EncodeContext(ctx, idx, gin.WithEncodeProfile(gin.EncoderProfileBoundedMemoryUncached))         // one call

What does not change: the output bytes, the wire format (v11), the two existing profiles and their constant values, gin.go, the CLI, the decoder. There is no new option, no new GINConfig field and no concurrency limit.

Two notes for the reviewer:

  • The issue says "closed after it". The code does not call Close(). In klauspost v1.20.0, Close() returns at once for an encoder used only through EncodeAll, and that path starts no goroutines. The memory is free when no pointer to the encoder remains.
  • A decoded index always carries the default profile, because the profile is never serialized. A job that loads an index and writes it again must pass WithEncodeProfile on that call. The godoc and README now say this.

Evidence

Level 15, darwin/arm64 (Apple M2 Max), GOMAXPROCS=4, klauspost v1.20.0, -benchtime=10x. Full table in docs/encoder-profile-benchmarks.md.

Profile Fixture Allocated per call Retained after the call Time per call
bounded-memory (cached) small 44.4 MB 42.2 MB 1.0 ms
bounded-memory-uncached small 44.7 MB 0 MB 3.6 ms
bounded-memory (cached) highcard 46.0 MB 42.5 MB 27.7 ms
bounded-memory-uncached highcard 50.0 MB 0 MB 31.0 ms
  • Before: no way to encode with zero retained memory without building the GINc payload by hand.
    After: make test passes, 1244 tests, 1 skipped (testdata/test.parquet is absent, same on main). go test -race -run 'EncoderProfile|Uncached|MixedProfiles' . passes.

Issue acceptance:

  • Byte-identical to the default profile at every level in encoderProfileLevels, single-block and multi-block: TestEncoderProfileBoundedOutputIdenticalToDefault now runs both non-default profiles.
  • No encoder stays reachable: TestEncoderProfileUncachedDoesNotRetainEncoder (live heap grows less than 8 MiB, and a control with the cached profile must grow more than 20 MiB), plus TestEncoderProfileUncachedLeavesCacheEmpty.
  • Doc row: new section "Uncached bounded-memory profile". git diff main -- docs/encoder-profile-benchmarks.md removes 0 lines.
  • Wire format: TestEncoderProfileUncachedWireFormat.

Mutations I applied by hand, then reverted, to check that the tests can fail:

Mutation in serialize.go Result
Send the uncached profile through the cache cache-empty, encode-paths, concurrent and heap tests fail
Keep the uncached encoder in a package variable heap test fails: retained 44225872 bytes after encode, want under 8 MiB
Build the uncached encoder with default options TestEncoderProfileBoundedUsesSingleWorker/bounded-memory-uncached fails

Lint: GOTOOLCHAIN=go1.25.5 make lint reports 0 issues. Plain make lint on my machine (go1.27.1) fails with a typecheck error in logging/attrs.go. main fails the same way, so it is the local linter build, not this change.

Not covered: no test calls RebuildWithIndex or the S3 sidecar writer with the new profile. Neither had a profile test before. Both pass options to the same EncodeWithLevelContext call.

Merge Danger

Door: two-way until a release tag, one-way after

The profile is opt-in, so a revert before the next tag affects no caller. After a release, the constant name EncoderProfileBoundedMemoryUncached and its value 2 are public API.

Blast Radius: opt-in

Callers that do not select the new profile run the same code as before, with one extra integer comparison per encode. A caller that selects it and encodes from N goroutines at once builds N encoders, about N x 44 MB allocated at level 15. The library sets no limit, by decision. The docs state the cost.

Also in the diff: the BenchmarkEncoderProfile retained metric is now a signed difference taken after two GC cycles, for all profiles. The old code subtracted unsigned values and could underflow. Planning records for this task are under .planning/quick/261006-hgl-*, including the question ledger and the 14 expectations the change was verified against.

oss-amikos and others added 9 commits October 6, 2026 12:59
…ile (#83)

- build a one-worker, low-memory zstd encoder per call and drop it
- bypass the shared encoder cache, so no encoder stays live after the call
- output stays byte-identical, wire format unchanged

Co-Authored-By: Claude <noreply@anthropic.com>
- bench the uncached profile in BenchmarkEncoderProfile
- clamp retained heap delta at 0 to avoid uint64 underflow
- append a measured section to docs/encoder-profile-benchmarks.md

Co-Authored-By: Claude <noreply@anthropic.com>
…GELOG (#83)

Co-Authored-By: Claude <noreply@anthropic.com>
…rge profile tests (#83)

sharedZstdEncoder is cache-only again. encodeWithLevel builds the
uncached encoder itself. The uncached profile now runs through the
existing bounded-memory tests instead of copies of them.

Co-Authored-By: Claude <noreply@anthropic.com>
…ile (#83)

sharedZstdEncoder rejects the uncached profile, so no caller can cache
it by mistake. The heap test warms up with the cached profile, which
lets it catch an encoder retained outside the cache. The benchmark
reports a signed heap difference in place of a value clamped at 0, and
the doc table has the numbers from a new run. Docs say that a decoded
index carries the default profile.

Co-Authored-By: Claude <noreply@anthropic.com>
…file (#83)

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
@oss-amikos
oss-amikos merged commit 6b5b690 into main Oct 6, 2026
11 checks passed
@tazarov
tazarov deleted the issue-83-bounded-memory-no-retention branch October 6, 2026 11:01
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.

[PERF] Bounded-memory encoder option that does not retain the encoder between calls

1 participant