Skip to content

zstd: avoid unused BuildDict encoder allocation - #1147

Merged
klauspost merged 1 commit into
klauspost:masterfrom
snissn:zstd-builddict-lazy-explicit-encoder
May 7, 2026
Merged

klauspost merged 1 commit into
klauspost:masterfrom
snissn:zstd-builddict-lazy-explicit-encoder

Conversation

@snissn

@snissn snissn commented May 6, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Avoid constructing a bestFastEncoder in BuildDict when callers provide an explicit BuildDictOptions.Level.

The previous code allocated the default best-compression encoder before checking the requested level. For explicit levels, that encoder was immediately overwritten by eOpts.encoder(), so its large tables were allocated and discarded without being used.

This delays the best encoder construction until the default Level == 0 path, where it is still used. The PR also adds:

  • TestBuildDictLevelPathsRoundTrip for default and explicit-level dictionary builds.
  • BenchmarkBuildDictLevelPaths to report the allocation difference between those paths.

Evidence

Correctness validation:

go test ./zstd

Allocation benchmark included in this PR:

go test ./zstd -run '^$' -bench '^BenchmarkBuildDictLevelPaths$' -benchmem -benchtime=5x -count=5

Environment: Go 1.25.5, darwin/arm64, Apple M3.

For the baseline, the benchmark/test additions were run on upstream/master without the zstd/dict.go implementation change.

Explicit SpeedFastest dictionary build:

Branch B/op allocs/op
upstream/master ~37,137,232 82
this PR ~1,477,456 81

That is about 35.66 MB/op less allocation and one fewer allocation for explicit-level dictionary builds on this fixture. The default Level == 0 path remains around 72.26 MB/op, as expected, since it still constructs the best-compression encoder.

Summary by CodeRabbit

  • Tests
    • Added comprehensive test coverage for dictionary building with different encoder level configurations and round-trip validation.

BuildDict initialized a bestFastEncoder before checking whether BuildDictOptions.Level was set. When callers requested an explicit level, that encoder was immediately replaced by eOpts.encoder(), so the large best-compression tables were allocated and discarded without being used.

Delay bestFastEncoder construction until the default-level path that actually uses it. This keeps the Level == 0 behavior unchanged while avoiding the unused encoder allocation for explicit levels.

Add round-trip coverage for both BuildDict level paths and a benchmark that reports the allocation difference between default and explicit-level dictionary builds.
Copilot AI review requested due to automatic review settings May 6, 2026 23:13
@coderabbitai

coderabbitai Bot commented May 6, 2026 •

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 1082fe27-3570-4f61-834a-5502fb682272

📥 Commits

Reviewing files that changed from the base of the PR and between 28ae503 and d2a08cf.

📒 Files selected for processing (2)
  • zstd/dict.go
  • zstd/dict_test.go

📝 Walkthrough

Walkthrough

The BuildDict function in zstd/dict.go refactors its encoder initialization by separating the enc variable declaration from its initial assignment. New test coverage validates round-tripping behavior with default and explicit SpeedFastest encoder levels.

Changes

BuildDict Encoder Initialization Refactoring

Layer / File(s) Summary
Implementation Refactoring
zstd/dict.go
BuildDict refactors encoder initialization: introduces var enc encoder declaration and assigns bestFastEncoder to it, with the same assignment repeated in the o.Level == 0 branch when the conditional path is not taken.
Test Coverage
zstd/dict_test.go
New fixture helper, round-trip test, and benchmark validate BuildDict behavior across default and explicit SpeedFastest encoder levels, ensuring dictionary correctness during encoding and decoding operations.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: avoiding unused encoder allocation in BuildDict when explicit levels are provided, matching the core optimization in the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

This comment was marked as duplicate.

@klauspost klauspost left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

lgtm

@klauspost
klauspost merged commit fdcc9b6 into klauspost:master May 7, 2026
25 of 26 checks passed
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.

3 participants