compute: a shared-trace primitive for cross-thread arrangement reads - #38386
compute: a shared-trace primitive for cross-thread arrangement reads#38386antiguru wants to merge 4 commits into
Conversation
fda6a81 to
ebd007b
Compare
ebd007b to
870f336
Compare
870f336 to
b508d89
Compare
…nt sharing (MaterializeInc#38396) Replaces MaterializeInc#37881, whose head branch lives on a fork and so cannot be the base of a stacked PR in this repository. Same commits, same tree, on an upstream branch instead. This is the root of the stack MaterializeInc#38386 through MaterializeInc#38393, which splits MaterializeInc#37770. ### Motivation Cross-runtime arrangement sharing (the two-runtime read-isolation work, MaterializeInc#37770) needs batches readable from a thread other than the one maintaining the trace. Differential's default spines reference-count batches with `Rc`, which is worker-local. ### Description Introduce `mz_row_spine::ArcBatch`, a local newtype around `Arc<B>` that carries differential's batch traits (the orphan rule forbids the blanket impl on a bare `Arc<B>`), and switch the production spines and their builders — `RowRowSpine`, `RowValSpine`, `RowSpine`, `ValRowSpine`, `ColValSpine`, `ColKeySpine` — from `Rc`/`RcBuilder` to `ArcBatch`/`ArcBuilder`. An `Arc`-backed batch whose contents are `Send + Sync` can be read across threads, which `Rc` cannot do. Only the batch handle becomes atomic; the batch contents are unchanged, so the cost is a marginally more expensive refcount. Also adds generic `ArcOrdVal`/`ArcOrdKeySpine` aliases for callers outside `mz_compute`, adapts batch-size logging (`log_arrangement_size_inner`) to reach through the newtype to the inner `Arc`, and switches the storage sink trace to the `Arc`-backed spine. Builds against released differential-dataflow 0.25 with no fork or `[patch.crates-io]`. ### Verification `cargo check --workspace` passes with no `Cargo.lock` churn. `relations.slt`'s golden is rewritten because the spine type name appears in operator names. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
QA LLM Review1. MEDIUM -- a live import permanently pins the published arrangement's physical compaction
The read hold DetailsMeasured on a Registration id 2 is the Cost is unbounded rather than constant: retractions in stranded Suggested fix, verified against the same probe (chain folds to 5, exactly matching the control, and the hold tracks to if let Some(hold) = hold.as_mut() {
hold.set_logical_compaction(acknowledged.borrow());
+ hold.set_physical_compaction(acknowledged.borrow());
}
|
|
Confirmed and fixed in 285af14. The measurement reproduces exactly: 39 batches against an unimported control's 5, over 40 seals. Both of the report's points were real, and the second one is the root cause rather than a separate cleanup. A handle carries two physical frontiers that are not interchangeable. The one it reports through The import's read hold hit this on both counts: it is a clone, so it registered at The fix keeps the two frontiers in separate fields, has Two regression tests, each verified red without its half of the fix:
The The stack above this PR is rebased and pushed. Full run at the tip: 124 tests, workspace (Posted by Claude Code.) |
Several modules in `mz-compute` carry test modules many times the size of the production code they cover, so the code has to be scrolled past to read. `src/cluster-controller` already uses the out-of-line pattern, where `#[cfg(test)] mod tests;` points at a sibling `tests.rs`. This records that as the crate convention, with a threshold so it is decidable rather than a matter of taste. Out-of-line tests still reach private items through `super::`, so moving a module needs no visibility changes. Worth landing before #38386 and the seven PRs stacked on it, which follow this rule and between them move about 5,900 lines of in-file test modules out of line. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
A handle carries two physical frontiers, and they are not interchangeable. The one it reports through `get_physical_compaction` is seeded at the published `since`, because a reported frontier may never lead the chain coverage. The one it holds is seeded at that coverage, because a merge spanning the coverage destroys the boundary the reader was seeded with. `Clone` and the setter both wrote the reported frontier into the hold, which silently lowers it. Since the accumulation is a meet, one such registration is a floor under every other hold, so the published spine stops merging: batches pile up in `Spine::pending`, one per seal, for as long as that registration lives. The cost is unbounded rather than constant, since retractions in stranded batches never consolidate and every `cursor_through` builds a `CursorList` over all of them. An import's read hold hit this on both counts. It is a clone, so it registered at `since`, and it advanced only on the logical axis, so nothing ever raised it. Measured against an unimported control over 40 seals: 39 batches against 5. Keep the two frontiers in separate fields, have `Clone` inherit the hold, have the setter join into both, and advance the import's hold on both axes. `acknowledged` is the right value for the physical axis too: it is exactly the frontier below which that import will never cut again. Reported by the QA LLM review on #38386. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
285af14 to
e953639
Compare
An arrangement is normally readable only from the timely worker that maintains it, because its batches are `Rc`-backed and its trace handle is neither `Send` nor `Sync`. This adds a publication point that carries `Arc`-backed batches together with the trace's `since` and `upper`, so a reader on any thread can mint a `Send` handle for the same arrangement and import it as a snapshot at a chosen `as_of`. Nothing in the crate calls it yet, so the module is inert: it compiles, its unit tests exercise publish, import, seal, and compaction holdback, and no rendered dataflow reaches it. A publication point is differential's `TraceBox` for readers that are not agents of the trace. It accumulates their holds in a `MutableAntichain` per axis and each handle adjusts that accumulation as a delta, the way a `TraceAgent` does, which costs the times that changed rather than a walk over every hold. Two special cases go with it: an empty request contributes nothing instead of having to be filtered out, and there is no zero-holds case to fall back from. The standing hold and the publisher's own hold at the chain coverage are ordinary holds in those accumulations, so a shared arrangement compacting no faster than the slowest runtime's command stream follows from a registered hold rather than from an invariant asserted after the fact. The controller's own frontier stays out of the accumulation. It is another agent's hold on the same trace, so it belongs to the meet the trace already computes, which is what the publisher publishes as `since`. The concrete `SharedOks*`/`SharedErrs*` type aliases live here rather than alongside the registry that will consume them. They name a shared-trace handle over `RowRowSpine` and `ErrSpine` and mention no registry type, so this is where they belong. `Published::diagnostics`, `note_writer_logical`, and `note_standing_hold` are `pub` like the rest of the type's accessors. Scoping them to the crate would make them unreachable for dead-code analysis while the only callers are the tests. Tests are out of line in `shared_trace/tests.rs`, per the convention in `src/compute/AGENTS.md`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A handle carries two physical frontiers, and they are not interchangeable. The one it reports through `get_physical_compaction` is seeded at the published `since`, because a reported frontier may never lead the chain coverage. The one it holds is seeded at that coverage, because a merge spanning the coverage destroys the boundary the reader was seeded with. `Clone` and the setter both wrote the reported frontier into the hold, which silently lowers it. Since the accumulation is a meet, one such registration is a floor under every other hold, so the published spine stops merging: batches pile up in `Spine::pending`, one per seal, for as long as that registration lives. The cost is unbounded rather than constant, since retractions in stranded batches never consolidate and every `cursor_through` builds a `CursorList` over all of them. An import's read hold hit this on both counts. It is a clone, so it registered at `since`, and it advanced only on the logical axis, so nothing ever raised it. Measured against an unimported control over 40 seals: 39 batches against 5. Keep the two frontiers in separate fields, have `Clone` inherit the hold, have the setter join into both, and advance the import's hold on both axes. `acknowledged` is the right value for the physical axis too: it is exactly the frontier below which that import will never cut again. Reported by the QA LLM review on #38386. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
e953639 to
e295c50
Compare
| //! An arrangement is normally readable only from the worker that maintains it: its batches are | ||
| //! reference counted with `Rc` and its trace handle is `Rc<RefCell<..>>`, both pinned to one | ||
| //! thread. This module lets a worker publish an arrangement whose batches are `Arc`'d (and whose | ||
| //! contents are `Send + Sync`) through a *publication point*, from which readers on other threads | ||
| //! take consistent snapshots or import the arrangement into a second timely runtime. |
There was a problem hiding this comment.
This is outdated, all batches should be behind Arc now.
There was a problem hiding this comment.
Correct, all mz spines are Spine<ArcBatch<..>>. Rewrote the paragraph: what is pinned to one thread is the trace handle, TraceAgent being Rc<RefCell<..>>, not the batches.
Posted by Claude Code.
| //! Logical compaction decides which times stay *distinguishable*, physical compaction which batches | ||
| //! may *merge*. A reader needs distinguishability at its `as_of`, and a boundary at each frontier it | ||
| //! passes to `cursor_through`. It needs no boundary at its `as_of`: an import is seeded with the whole | ||
| //! chain and wrapped in `TraceFrontier`, which advances times rather than cutting. Conflating the two | ||
| //! is the mistake to avoid, and forwarding `since` as the physical frontier is what stopped published | ||
| //! arrangements from merging at all. | ||
| //! | ||
| //! Two of the holds have no reader behind them, and both exist because a reader registers only once | ||
| //! its dataflow is built, while the agent's setter joins and so only ever advances. The *standing | ||
| //! hold* tracks the frontier the importing runtime has applied, keeping the agent at or below every | ||
| //! `as_of` that runtime can still present. The *coverage hold* is its physical counterpart: a reader | ||
| //! registering on the next activation seeds at the current chain coverage and needs a boundary there. | ||
| //! | ||
| //! The sharing machinery lives entirely in Materialize, so it builds against a released | ||
| //! differential-dataflow rather than a fork. Publishing is exposed as the [`PublishArrangement`] | ||
| //! extension trait, since Materialize cannot add inherent methods to differential's foreign | ||
| //! `Arranged` type. Cross-thread batch sharing rests on the local `mz_row_spine::ArcBatch` newtype, | ||
| //! not on any differential-side `Arc` batch impls. |
There was a problem hiding this comment.
This mixes facts with outdated experience. Rewrite, or drop.
There was a problem hiding this comment.
Dropped the paragraph. The extension-trait rationale is already owned by PublishArrangement's own doc, and the rest was fork and ArcBatch history that no longer describes the code.
Posted by Claude Code.
| /// | ||
| /// The `chain`, `since`, and `upper` are always updated together under the lock, so every reader | ||
| /// observes a frontier-consistent view. | ||
| struct SharedTraceState<Tr: TraceReader> { |
There was a problem hiding this comment.
Would an alternative be to make Spine/traces shareable across thread boundaries? Or wrap it in an Arc<Mutex<...>>? It seems we need to duplicate a bunch of logic for the sole reason of enabling sharing.
There was a problem hiding this comment.
Considered, but neither alternative removes the layer.
Spine<ArcBatch<..>> most likely is Send. TraceAgent is not: it holds Rc<RefCell<TraceBox<Tr>>>, an Rc listener-queue list, and an Activator. So the only shareable thing is the Spine itself, wrapped by us.
Wrapping it in Arc<Mutex<..>> puts every reader on the writer's critical path. set_physical_compaction can run an unbounded merge synchronously, so a reader acquiring a cursor would block the publishing worker for the duration of a merge, and a merge would block every reader. The publication point exists precisely so readers touch an immutable Arc chain and never the writer's structure.
It also would not remove the duplicated logic:
- A cross-thread reader is not an agent of the trace, so it cannot register in the
TraceBox.Spine::batches_throughasserts the cut is at or beyond the spine's physical frontier, a precondition that holds only for agents, so a separate hold accumulation for non-agent readers is needed either way. - The replay path needs a
Sendqueue and aSyncActivator.TraceAgent::new_listenerusesRc<(Activator, RefCell<VecDeque<..>>)>.
What was genuinely duplicated was the per-hold bookkeeping, and that is gone: the two BTreeMaps of per-registration holds, the registration id on a handle, and coverage_hold. What remains of the TraceBox analogue is adjust plus two one-line movers.
Posted by Claude Code.
| /// The accumulation alone cannot say which reader holds what, and a refusal diagnostic needs | ||
| /// exactly that: "a hold sits at `f`" and "no hold exists and the frontier happens to be `f`" | ||
| /// are the difference between an import that is protected and one that is not. |
There was a problem hiding this comment.
Why do we need to know which reader holds what?
There was a problem hiding this comment.
We do not. The map existed only to recover a registration's previous frontier so the accumulation could be adjusted by a delta, and the handle already owns that value. The setters now pass previous explicitly and both maps are gone, along with the registration id on a handle: ids now serve only importer queues.
The diagnostic argument in the doc did not survive contact either. MutableAntichain distinguishes "a hold sits at f" from "no hold and the frontier happens to be f" by being empty in the second case, which is what the two tests now assert.
Posted by Claude Code.
| /// Per-registration physical holds: the lowest frontier each reader may still cut at. | ||
| /// | ||
| /// This is the path a shared reader's request travels, and a local consumer needs no equivalent: | ||
| /// `crate::render::join::mz_join_core` is an agent on its own trace, so its | ||
| /// `set_physical_compaction(acknowledged)` reaches the `TraceBox` directly. | ||
| /// | ||
| /// An entry starts at the chain coverage at registration, never at `since` and never at `as_of`: | ||
| /// `acknowledged` is initialised to that coverage in `SharedTraceHandle::import_snapshot_at` and | ||
| /// only advances, so no cut ever happens below it. |
There was a problem hiding this comment.
Same treatment, physical_holds is gone for the same reason. What survives is the accumulated MutableAntichain on each axis and nothing per-registration.
Posted by Claude Code.
| for batch in state.chain.iter() { | ||
| // A batch whose lower is beyond the cut, and everything after it in the totally | ||
| // ordered chain, lies past `upper`. Empty batches never carry updates to read. | ||
| if timely::PartialOrder::less_equal(&upper, &batch.lower().borrow()) { |
There was a problem hiding this comment.
Let's enforce that Time: TotalOrder so this claim is actually true.
There was a problem hiding this comment.
Added. Tr::Time: TotalOrder is now a bound on the TraceReader impl and on the import_snapshot_at impl block. Both instantiations are over mz_repr::Timestamp, so nothing else moved.
Posted by Claude Code.
| fn batch_min<Tr: TraceReader>() -> Tr::Time { | ||
| <Tr::Time as timely::progress::Timestamp>::minimum() | ||
| } |
There was a problem hiding this comment.
Let's inline this function.
There was a problem hiding this comment.
Inlined, batch_min is gone. The two remaining call sites spell <Tr::Time as timely::progress::Timestamp>::minimum().
Posted by Claude Code.
| /// An owned, consistent snapshot of a published arrangement: an immutable chain plus its frontiers. | ||
| /// | ||
| /// Test-only, the result of [`SharedTraceHandle::snapshot_at`]. Holding it pins the chain's batches, | ||
| /// keeping their memory alive even as the publishing worker merges. | ||
| #[cfg(test)] | ||
| pub(crate) struct TraceSnapshot<Tr: TraceReader> { | ||
| chain: Vec<Tr::Batch>, | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| impl<Tr: TraceReader> TraceSnapshot<Tr> { | ||
| /// A cursor merging the snapshot's batch cursors, with the batches as its storage. | ||
| pub(crate) fn cursor(&self) -> (CursorList<<Tr::Batch as Navigable>::Cursor>, Vec<Tr::Batch>) | ||
| where | ||
| Tr::Batch: Navigable, | ||
| { | ||
| cursor_list(self.chain.clone()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we move this into the test module?
There was a problem hiding this comment.
Moved, and took the rest of the test-only surface with it. TraceSnapshot, its cursor, SharedTraceHandle::snapshot_at, and the four Published accessors (logical_holds, physical_holds, chain_len, standing_hold) now live in shared_trace/tests.rs, which reaches the private fields through super::. shared_trace.rs has no cfg(test) items left.
Posted by Claude Code.
| /// # Why replay rather than a one-shot emit | ||
| /// | ||
| /// The returned [`Arranged`]'s `stream` and `trace` must stay consistent: the trace is the | ||
| /// accumulation of the stream, and their frontiers advance together. A differential join relies | ||
| /// on this (it computes `A.stream x B.trace + B.stream x A.trace`, counting each match once only | ||
| /// when the trace never runs ahead of the stream). Driving the output capability off the replayed | ||
| /// `Frontier` instructions keeps `stream.frontier == trace.upper`. A one-shot emit that shipped | ||
| /// the whole chain and then dropped straight to the empty frontier would leave the pre-populated | ||
| /// shared trace ahead of the stream, and the join would read the same record from both and double | ||
| /// it. | ||
| /// | ||
| /// For a single-time interactive read pass `until = as_of.step_forward()`: the capability then | ||
| /// drops once the trace's frontier passes `as_of`, so the one-shot result completes. An empty | ||
| /// `until` performs no bounding and the import stays live with the trace. |
There was a problem hiding this comment.
It seems to me this is stating what the caller would expect to be true? until can be the empty frontier, so it's clear this must continually replay updates.
There was a problem hiding this comment.
Dropped the sentence about the empty until. Kept the first half, since the until = as_of.step_forward() idiom for a one-shot read is the thing a caller has to be told.
Posted by Claude Code.
| // A batch the seed already covers. The chain is read from the | ||
| // trace, which can hold a batch the arrangement stream has not | ||
| // delivered yet, so the publisher will push that same batch as a | ||
| // live instruction on a later activation. Emitting it twice would | ||
| // double count it, and its hint sits below the frontier the seed | ||
| // already claimed, which `delayed` panics on. | ||
| if !draining_seed | ||
| && timely::PartialOrder::less_equal( | ||
| &batch.upper().borrow(), | ||
| &seed.borrow(), | ||
| ) | ||
| { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Why is there ever a batch that would be emitted twice? This seems odd.
There was a problem hiding this comment.
It is real, and the comment buried the reason. The two sources are different.
The seed comes from the trace, via agent.map_batches, at the moment the importer registers. The live instructions come from the arrangement stream. The trace holds a sealed batch a scheduling round before the stream delivers it, so a batch already in the seed can arrive again as a live instruction. Emitting it twice would double count it, and its hint sits below the frontier the seed already claimed, which caps.delayed panics on.
The filter stays, with the comment rewritten to lead with the trace-versus-stream lag rather than with the symptom. live_batch_covered_by_the_seed_is_dropped covers it.
Posted by Claude Code.
Review feedback on #38386. A handle carried two physical frontiers, one it reported and one it held. Reporting the chain coverage satisfies the only consumer that reads the frontier back, `mz_join_core`, whose assertion compares it against the coverage it derives from `map_batches`. So the two collapse into one field seeded at the coverage, and the class of bug the split was guarding against stops being expressible. The publication point also kept a `BTreeMap` of per-registration holds on each axis, solely to recover the previous value when computing a delta into the accumulation. The handle already owns that value, so the maps go and the setters pass `previous` explicitly. A handle no longer needs a registration id at all; ids now serve only importer queues. `coverage_hold` goes the same way: the publisher falls back to the chain coverage when the accumulation is empty, which is the shape the logical axis already used. Test-only surface moves into `shared_trace/tests.rs`, which reaches private items through `super::`. `snapshot_at` gains a deadline so a wedged publisher fails with the frontier it stalled on rather than hanging. `Tr::Time: TotalOrder` is now a bound on the `TraceReader` impl, so `batches_through` stopping at the first batch beyond the cut rests on a stated property rather than a comment. The module is `pub(crate)` with an explicit `allow(dead_code)`, rather than `pub` to keep dead-code analysis quiet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Complexity pass over the module, no behaviour change. `TraceReader` already declares `type Time: Timestamp + Lattice`, and timely's `Timestamp` implies `Clone`, `Send`, and `'static`. Every `Tr::Time: Lattice + Clone` clause here was therefore vacuous. Dropping them leaves only the bounds that constrain something, `TotalOrder` and `Sync`, and removes a real gotcha: `Drop` could not repeat the vacuous bound, so it reached past the state's own API to adjust the accumulations directly. It now calls the movers like every other caller. `SharedTraceHandle::writer_logical` had no caller, and `Published::diagnostics` already returns the same frontier from the publication point, which its own doc argues is the right place to read it from. `PublishArrangement::adopt_named` had no caller either; its only invocation was `adopt` forwarding a literal. Comments: several facts were owned by two or three places at once. The choice of physical seed now lives only at `register_at`, the standing hold's seed only at `adopt`, the pairwise-peers invariant only on `SharedTrace::peers`, and the lost-wakeup argument only at the `on_seal` call. Each remaining copy points at the owner. Also dropped two references that no longer resolve, one to a helper that moved into the test module and one chronology clause. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
First of eight PRs splitting #37770. Stacks on #38396. Tracked by CPU-215.
An arrangement is normally readable only from the timely worker that maintains it, because its batches are
Rc-backed and its trace handle is neitherSendnorSync. This adds a publication point carryingArc-backed batches together with the trace'ssinceandupper, so a reader on any thread can mint aSendhandle for the same arrangement and import it as a snapshot at a chosenas_of.A publication point is differential's
TraceBoxfor readers that are not agents of the trace. It accumulates their holds in aMutableAntichainper axis, and each handle adjusts that accumulation as a delta the way aTraceAgentdoes. The standing hold and the publisher's own hold at the chain coverage are ordinary holds in those accumulations, so a shared arrangement compacting no faster than the slowest runtime's command stream follows from a registered hold rather than from an invariant asserted after the fact. The controller's own frontier stays out, since it is another agent's hold on the same trace and so belongs to the meet the trace already computes, which is what the publisher publishes assince.Inert: nothing in the crate calls it. The module is
pubso that its accessors are reachable for dead-code analysis while the only callers are its own tests.PublishArrangementrequiresTr::Batch: Send + Syncand the tests instantiate it overRowRowSpine, so Arc-backed spines are a compile prerequisite rather than a preference. This PR retargets tomainonce #38396 merges.