Skip to content

timely-util: spill young chunk generations uncompressed - #38253

Merged
DAlperin merged 5 commits into
mainfrom
dov/chunk-compress-floor
Aug 24, 2026
Merged

timely-util: spill young chunk generations uncompressed#38253
DAlperin merged 5 commits into
mainfrom
dov/chunk-compress-floor

Conversation

@DAlperin

@DAlperin DAlperin commented Aug 17, 2026

Copy link
Copy Markdown
Member

Motivation

Spilled chunk bodies currently compress with lz4 unconditionally. Fresh chunks (generation depth 0) are usually consumed by their first merge moments after they spill, so compressing them buys little swap savings and pays encode plus decode on the hottest path. The August benchmark campaign measured this as roughly half of the spill-on co-tenant tail at 50cc: sweeping the floor from compress-everything to never-compress cut the co-tenant probe p99 from 16.6 s to 7.5 s under an ~18k rows/s upsert load (details in the campaign notes, results reproduced across two runs).

This adds a generational compression floor: bodies below column_chunk_compress_min_depth (default 1, replica-scoped) spill through a new identity codec in mz_ore::pool. They stay budget-accounted and swap-backed, but encode and decode reduce to copies. Keeping young bodies out of the pool entirely was rejected because exempted bytes would be invisible to the budget.

Tips for reviewer

  • The codec choice happens once at spill time (codec_for_depth), so a flag change affects newly spilled bodies only.
  • 0 restores compress-everything, 255 is effectively never-compress.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

@DAlperin DAlperin changed the title dov/chunk compress floor timely-util: spill young chunk generations uncompressed Aug 17, 2026
@DAlperin
DAlperin force-pushed the dov/chunk-compress-floor branch from b293114 to aa40adb Compare August 17, 2026 19:31
@DAlperin

Copy link
Copy Markdown
Member Author

This also gives us an easy way to disable compression altogether.

@DAlperin
DAlperin marked this pull request as ready for review August 18, 2026 21:44
@DAlperin
DAlperin requested review from a team as code owners August 18, 2026 21:44

@antiguru antiguru left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems good! Thank you

Comment on lines +142 to +147
/// Set or unset a thread-scoped depth-floor override, taking precedence over
/// [`set_compress_min_depth`]. For tests and benches, which run concurrently
/// and must not race on the process-global floor.
pub fn set_compress_min_depth_override(depth: Option<u8>) {
COMPRESS_MIN_DEPTH_OVERRIDE.with(|cell| cell.set(depth));
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not in love with test-only code compiled into the release binary. Could we mark this #[cfg(test)], or is it used by a different crate? The other solution is to have a separate test file, which runs as a separate binary.

@def-

def- commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

QA LLM Review

1. MEDIUM -- Depth 0 does not mean "young": merge's pass-through paths never increment it

src/timely-util/src/columnar/chunk.rs:158

The default floor rests on "a fresh chunk is consumed by its first merge with certainty", but merge has two paths that carry a chunk forward without touching its depth, and both leave an already-spilled body in the pool untouched. For input whose successive chunks are key-disjoint and ascending, every chain merge takes one of those paths, so the whole batcher backlog sits at depth 0 for as long as it lives and spills uncompressed the entire time -- the opposite of what the floor is for.

Details

Where depth is preserved rather than incremented:

  • chunk.rs:522-529 — the disjoint fast path moves the lower front to the output verbatim. A ColumnChunk::Spilled passes through as-is: no read, no re-spill, no codec round-trip.
  • chunk.rs:566-573 — an untouched survivor is restored as its original spilled body at its original depth.
  • advance (chunk.rs:645, 655), settle (chunk.rs:785) and extract (chunk.rs:598) all take max, never +1. Only an overlapping merge increments, which the existing test at chunk.rs:1647 states outright: "pass-through keeps its depth".
  • The harness compounds it: ChunkMerger::merge (differential 0.25.1, trace/chunk/mod.rs:521-525) appends the entire remaining tail of the longer chain verbatim once the merge loop ends, so even overlapping workloads carry depth-0 chunks forward.

An append-only source keyed by a monotonically increasing id produces exactly the disjoint-ascending shape per worker: batch t covers keys strictly below batch t+1 whatever the hash routing, because the keys grow with time. Every chain merge is then a pass-through and nothing in the batcher ever reaches depth 1. Since depth is now the codec key, those bodies stay identity-coded for the whole life of the batcher — during snapshot hydration, that is the entire hydration, the window the pool exists for. At the ~5.6x ratio the tier is sized against (COLUMN_PAGED_BATCHER_POOL_RSS_TARGET_FRACTION, dyncfgs.rs), the compressed tier holds ~5.6x less logical data for those arrangements and the swap footprint grows correspondingly.

Two directions: increment depth on the pass-through and untouched-survivor paths too (a chunk that survives a merge round has demonstrably outlived one merge cadence, which is the property the floor actually wants), or key the codec on residence rather than merge-generation — e.g. compress on the second spill of a body, or when the pool reports the chunk survived an eviction.

@def-

def- commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

QA LLM Review

1. MEDIUM -- Aging is silently dropped whenever the body is shared, so on the compaction path only the expensive branch of survive_merge ever runs

src/timely-util/src/columnar/chunk.rs:434

The cheap metadata bump requires Rc::try_unwrap to succeed, but the trace's compaction merger feeds merge clones of the source batches' chunks, so a spilled body is always shared there and the bump is discarded. The only branch that lands on that path is the floor crossing, which reads and re-inserts the whole body just to carry a one-byte increment, and advance reads that fresh body back out and frees it in the same call.

Details

ChunkBatchMerger::work (differential-dataflow 0.25.1, trace/chunk/mod.rs:628 and :632) refills in1/in2 with source.chunks[i].clone(), and the spine holds both source batches until the merge finishes, so every Spilled chunk merge sees there has refcount >= 2 and chunk.rs:439's Err(body) arm hands the chunk back at its original depth.

The crossing branch at chunk.rs:425 ignores sharing and re-spills regardless: a full pool read plus a fresh slot insert, discarding whatever backing the original had already paid for. In the same work iteration C::advance reads every chunk of merged into one column (chunk.rs:705-715), so the new body dies before anything can benefit from its codec. Net cost is one extra whole-body round trip through the pool per batch chunk at its first compaction, on the merge path this PR set out to keep free of codec work, for a depth increment the metadata bump would have carried for free.

Past the crossing nothing ages on that path at all: a key-disjoint arrangement's bodies pin at depth 1 forever, so eviction bands 2 and 3 (DEPTH_BANDS = 4, src/ore/src/pool.rs:177) stay empty for exactly the workload aging was added for. With the floor configured at 2 or more the crossing is unreachable (0 < 2 && 1 >= 2 is false), so those bodies never leave the identity codec. merge_survivor_crosses_compression_floor builds its inputs directly and so only exercises unshared bodies, where both branches behave identically.

Fix: depth is chunk metadata, not body metadata. The pool's ChunkHints are insert-time and immutable, and body.depth is read nowhere but depth() and the crossing test, so moving it into the variant (Spilled(Rc<SpilledBody<D>>, u8), matching Resident) makes the bump unconditional and free. The re-spill can then be skipped when Rc::strong_count(&body) > 1: another holder keeps the original body under its original codec, so re-spilling a clone cannot change what that holder stores, and the clone is about to be rewritten anyway.

A chunk at generational depth d is rewritten with frequency proportional
to 2^-d under geometric merging, so compressing a shallow generation buys
pool bytes back for only a short stay at a guaranteed near-term codec
round-trip: fresh (depth 0) chunks are consumed by their first merge with
certainty. Spill bodies below a configurable compression depth floor
(default 1) under a new identity ExtentCodec in mz_ore::pool instead:
still inserted into the pool, so they stay budget-accounted and
swap-backed like every extent, but encode and decode reduce to copies.

Keeping young generations out of the pool entirely was rejected: exempted
bytes are invisible to the pool budget, and coalesced carries at 2 MiB
each across many operators could accumulate unbudgeted resident state
exactly when the system is busiest.

The floor is wired through the replica-scoped
column_chunk_compress_min_depth dyncfg alongside the other chunk knobs.
The compression floor read depth 0 as "young, about to be rewritten", but
merge carries chunks forward without incrementing depth on two paths: the
disjoint fast path moves the lower front to the output verbatim, and an
untouched survivor is restored as its original body. Input whose successive
chunks are key-disjoint and ascending, which is what an append-only source
keyed by a monotonic id produces per worker, takes one of those paths on
every chain merge, so the whole backlog sits at depth 0 and spills
identity-coded for as long as it lives. That is the case the floor was least
meant to cover: the bodies are long-lived, and the pool holds them
uncompressed for the entire hydration.

Depth now counts merge cadences lived through rather than rewrites, so a
chunk a merge carries forward untouched ages a generation too. A spilled
body that crosses the compression floor by aging is re-spilled under the
compressing codec, since surviving a merge disproves the imminent-rewrite
premise that exempted it. Chunks consumed by their first merge, the case the
floor targets, never reach these paths and still skip codec work entirely; a
survivor pays one encode at the crossing and none after.
Depth lived on SpilledBody, so aging a spilled chunk meant mutating a shared
body: the bump needed Rc::try_unwrap and was dropped whenever it failed. The
trace's compaction merger feeds merge clones of a source batch's chunks and
holds the batch until the merge finishes, so every spilled chunk it sees is
shared and never aged. Only the floor crossing ran there, ignoring sharing:
a full pool read and a fresh insert to carry a one-byte increment, on a body
advance loads back out and frees in the same call. Past the crossing nothing
aged at all, pinning key-disjoint arrangements at depth 1 so the deeper
eviction bands stayed empty, and with the floor at 2 or more the crossing
was unreachable, leaving those bodies identity-coded for good.

Depth is chunk metadata, not body metadata: a body outlives the chunks that
share it, and aging must not depend on who holds the only reference. It
moves onto the variant next to the body, matching Resident, which makes the
bump free and unconditional. The re-spill is then skipped for a shared body,
since re-spilling one reference cannot change what another holder stores and
the merger rewrites its clone at once. Sole owners, which is what the merge
batcher's chains hold, still re-spill at the crossing and keep the floor's
compression behavior.

Depth and the pool's ChunkHints diverge after an in-place bump, since hints
are fixed at insert. Eviction banding therefore tracks aging only across
re-spills.
@DAlperin
DAlperin force-pushed the dov/chunk-compress-floor branch from 52010e4 to 020ead7 Compare August 24, 2026 14:47
@def-

def- commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

QA LLM Review

1. MEDIUM -- An unavailable pool permanently misses the compression-floor transition

src/timely-util/src/columnar/chunk.rs:442

When a sole spilled survivor crosses the compression floor while spill_pool() is None, this branch records the post-floor depth without re-spilling the identity-coded body. Every later survival sees was >= compress_min_depth(), so re-enabling spilling cannot retry the transition and a long-lived key-disjoint backlog remains uncompressed.

Details

This is reachable when spilling is toggled at runtime: existing handles remain valid after both spill gates are disabled, but spill_pool() returns None until either gate is enabled again. A depth-0 body that survives a disjoint merge during that interval returns as depth 1 here. Future survivals only bump 1 to 2 and beyond, while settle passes the spilled body through without recommitting it, so its codec never changes. This also contradicts the method's stated contract that an unavailable crossing "retries at the next survival." Preserve was in the None arm to retain that retry, or track the body's stored codec separately from its logical depth so aging and codec migration do not share one state variable.

…e missed

survive_merge decided a body wanted recompressing from a one-tick depth
transition, was below the floor and the bump lands at or past it. Nothing
recorded what a body was actually stored as, so any path that skipped the
tick consumed it: the next survival saw was >= floor and the body stayed
identity-coded for the rest of its life. Three ways in. With spilling
toggled off at runtime, existing handles stay valid but spill_pool returns
None, so a body that survives during that window ages past the floor without
migrating and never retries. A shared body skips for the same reason and
never retries once unshared. Lowering the floor, which is what an operator
reaches for under pool pressure, migrates nothing at all: every already
spilled body is long past the transition.

SpilledBody now records whether it was inserted under the compressing codec,
and survive_merge migrates when that flag is false and the chunk is at or
past the floor. The condition is a standing property of the body rather than
an edge, so a migration that cannot happen now is retried at the next
survival instead of lost, and a floor change applies to bodies that already
spilled. codec_for_depth returns the codec and the flag together from one
read of the floor, so the two cannot disagree when the floor moves under a
concurrent commit.

The migration is still skipped while a body is shared, since re-spilling one
reference cannot change what another holder stores, but skipping no longer
strands it.
@DAlperin
DAlperin merged commit 0481090 into main Aug 24, 2026
82 checks passed
@DAlperin
DAlperin deleted the dov/chunk-compress-floor branch August 24, 2026 15:25
DAlperin added a commit that referenced this pull request Aug 27, 2026
### Motivation

The upsert v2 (continual feedback) stash moves from its previous
representation onto `ChunkBatcher`/`ChunkSpine`: stashed updates live in
columnar chunks that participate in the buffer pool, and the feedback
arrangement drains through `UnloadChunk` one chunk at a time instead of
materializing whole batches. With `enable_upsert_paged_spill` on, stash
and arrangement state past the residency budget spills as compressed,
budget-accounted extents rather than raw kernel-swapped heap.

Benchmarked extensively on AWS (self-managed EKS, MSK source,
swap-enabled nodes), August 2026 campaign:

* Hydration under pressure, 25cc replica: at 4.5x state-to-memory,
v2-spill hydrates in 707 s vs 1187 s for v1 (RocksDB mem-env). At 9x:
1607 s vs 4607 s (v1 goes superlinear, v2 stays near-linear). At 13.5x,
v1 and v2-resident crash-loop at the pod swap ceiling and never
complete; v2-spill finishes in 2386 s with matching counts.
* Swap traffic: v2-spill moves roughly half the swap bytes of v1 at
every depth measured.
* Steady-state ingest freshness under the same pressure: statistically
identical across v1, v2-resident, and v2-spill.
* Unpressured regimes: v2-resident tracks v1 within ~10% everywhere;
spill costs appear only at a 50cc saturation edge and vanish one size up
(or under the two chunk patches below this PR in the stack).

The new representation is flagged: `enable_upsert_chunked_stash` (off in
production, on and randomized in CI) selects between the `ChunkBatcher`
representation above and the previous paged-columnar-merge-batcher +
`ValRowSpine` representation, which stays the production default while
the chunked flavor earns trust. The operator loop is shared; a small
`UpsertStashArm` trait carries the flavor-specific pieces (stash
batcher, feedback spine, flush, drain), and the arms are resolved from
the config set once at operator construction, so a dataflow keeps its
flavor for life. Spilling in either flavor remains gated by
`enable_upsert_paged_spill`.

### Tips for reviewer

* Stacked on #38253 and #38254 (the two chunk patches the campaign
produced); the interesting upsert logic is
`upsert_continual_feedback_v2.rs`.
* `enable_upsert_paged_spill` is replica-scoped and composes with
compute's spill gate as an OR on the process-wide pool.
* The unit-test harness runs every scenario under both stash flavors and
asserts they produce identical output, so the paged arm is exercised by
the same tests as the chunked one.

### Checklist

- [ ] This PR has adequate test coverage / QA involvement has been duly
considered. ([trigger-ci for additional test/nightly
runs](https://trigger-ci.dev.materialize.com/))
- [ ] This PR has an associated up-to-date [design
doc](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/README.md),
is a design doc
([template](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/00000000_template.md)),
or is sufficiently small to not require a design.
- [ ] If this PR evolves [an existing `$T ⇔ Proto$T`
mapping](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/command-and-response-binary-encoding.md)
(possibly in a backwards-incompatible way), then it is tagged with a
`T-proto` label.
- [ ] If this PR will require changes to cloud orchestration or tests,
there is a companion cloud PR to account for those changes that is
tagged with the release-blocker label
([example](MaterializeInc/cloud#5021)).
- [x] If this PR includes major [user-facing behavior
changes](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/guide-changes.md#what-changes-require-a-release-note),
I have pinged the relevant PM to schedule a changelog post.
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