[CORE-16902] compression: allocate and re-use a single WorkingMemory space for snappy compression - #31296
Conversation
Upstream `1.2.2` fixes a stale `SNAPPY_VERSION = (1, 1, 10)` constant that shipped in the `1.2.1` release's `BUILD.bazel`, which resulted in incorrect values into the generated `snappy-stubs-public.h` version macros.
Move `snappy-internal.h` into the exported headers of @snappy so that compression code can drive snappy's internal block compressor with caller-owned scratch memory.
`snappy`'s public one-shot API (`RawCompress()`) internally allocates a `~200KiB` `WorkingMemory` on every call, which can result in oversized allocation warnings, reactor stalls, and potentially OOMs for `redpanda`. Instead, drive `snappy`'s internal block compressor (`CompressFragment()`, from `snappy-internal.h`) with a per-shard `WorkingMemory` and staging buffer, allocated once at broker startup before the warning threshold is armed, mirroring the `zstd` & `lz4` workspace preallocations. A `static_assert` pins the `snappy` version to force re-auditing the internal API contracts on upgrade. A round-trip parity test verifies each framed block is byte-identical to `snappy::RawCompress()` output, so any semantic drift in the internals would be caught in CI. From an ad-hoc benchmark, not checked in, ``` ┌─────────────────────────┬───────────────────────────┬───────────────────────────┐ │ payload (1 MiB) │ old: time / allocs per op │ new: time / allocs per op │ ├─────────────────────────┼───────────────────────────┼───────────────────────────┤ │ json-like records │ 438.8 µs / 47.0 │ 436.0 µs / 28.0 │ ├─────────────────────────┼───────────────────────────┼───────────────────────────┤ │ repeated 512B string │ 44.8 µs / 39.0 │ 43.4 µs / 20.0 │ ├─────────────────────────┼───────────────────────────┼───────────────────────────┤ │ random (incompressible) │ 56.8 µs / 27.0 │ 56.0 µs / 20.0 │ └─────────────────────────┴───────────────────────────┴───────────────────────────┘ ```
There was a problem hiding this comment.
Pull request overview
This PR reduces per-call memory allocations during java_snappy compression by reusing a per-shard Snappy WorkingMemory (and staging buffer) instead of relying on Snappy’s public one-shot API that allocates large scratch space each call. It integrates workspace preallocation into broker startup and adds regression tests to ensure output parity with Snappy’s public API behavior.
Changes:
- Rework
snappy_java_compressorto drive Snappy’s internalCompressFragment()using a per-shard reusable workspace. - Preallocate the Snappy workspace on all shards during application startup to avoid large-allocation warnings on first use.
- Add boundary + differential tests to validate round-trip correctness and byte-for-byte parity vs the public Snappy API.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/v/redpanda/application.cc | Preallocates Snappy workspace on all shards during startup. |
| src/v/compression/tests/snappy_tests.cc | Adds boundary-size round-trip tests and a differential parity test vs Snappy public API. |
| src/v/compression/internal/snappy_java_compressor.h | Adds init_workspace() API for startup preallocation. |
| src/v/compression/internal/snappy_java_compressor.cc | Implements per-shard reusable Snappy working memory and block staging buffer; pins Snappy version via static_assert. |
| src/v/compression/BUILD | Adds dependency needed for Snappy internal/stubs headers. |
| MODULE.bazel | Bumps Snappy Bazel module version and applies a patch to export internal headers. |
| MODULE.bazel.lock | Updates lock entries for the new Snappy module version. |
| bazel/thirdparty/snappy-export-internal-h.patch | Exposes snappy-internal.h to dependents for internal API usage. |
Comments suppressed due to low confidence (1)
src/v/compression/tests/snappy_tests.cc:14
- New test helpers use std::array and std::min, but the file does not include / . Add these includes to avoid depending on indirect includes.
#include "base/units.h"
#include "bytes/iobuf.h"
#include "compression/internal/snappy_java_compressor.h"
#include "compression/snappy_standard_compressor.h"
#include "random/generators.h"
StephanDollberg
left a comment
There was a problem hiding this comment.
bit yuck but makes sense I guess
Agreed 😢 I have a PR upstream which should allow us to remove some of the hand-rolling of |
snappy's public one-shot API (RawCompress()) internally allocates a~200KiBWorkingMemoryon every call, which can result in oversized allocation warnings, reactor stalls, and potentially OOMs forredpanda.Instead, drive
snappy's internal block compressor (CompressFragment(), fromsnappy-internal.h) with a per-shardWorkingMemoryand staging buffer, allocated once at broker startup before the warning threshold is armed, mirroring thezstd&lz4workspace preallocations. This also saves on oness::temporary_bufferallocation per compression on our end.A
static_assertpins thesnappyversion to force re-auditing the internal API contracts on upgrade. A round-trip parity test verifies each framed block is byte-identical tosnappy::RawCompress()output, so any semantic drift in the internals would be caught in CI.From an ad-hoc benchmark, not checked in,
Along with this PR (which is a fix for now until
snappyoffers additional public APIs for re-using working memory buffers), I opened an issue upstream: google/snappy#248Also fixes CORE-15883.
Backports Required
Release Notes
Improvements
snappycompression.