The corgi backend, on more than one worker - #851
Merged
Conversation
The corgi backend's `arrange` used `Pipeline` and asserted `peers() == 1`, because a multi-worker run would have MIS-PLACED keys -- silently wrong, not slow. This replaces that assert with the exchange it was waiting for. `CorgiPact` / `CorgiDistributor` (corgi/exchange.rs) partition a container the way a columnar engine does, not the way timely's stock distributor does. There are no items to drain from a CorgiContainer -- it is four columns -- so taking it apart row-wise would undo the representation before the data left the worker. Instead: hash the key column once (corgi::hash, the same structural hash `present_key` prepends as a key's identifier lane), counting-sort row indices by destination, then `gather` each destination's rows into fresh contiguous columns. Contiguous is what the wire format wants; the sort is stable, so rows keep their order within a destination. A container whose rows all share a destination moves whole, with no gather at all. corgi/bytes.rs is the wire format: four length words, then the key and value columns through corgi's new byte codec (one memcpy per leaf column, no per-row framing) and the time and diff columns through columnar's Stash -- the encoder `T: Columnar` was already chosen for. Every section is a whole number of words, so a receiver can install the bytes rather than relocate them. The row backend reaches the wire through bincode, walking every Value of every row; that contrast is the point. Substituting the pact in `arrange` is the whole of multi-worker support: every other operator (linear, join, reduce, as_collection) is key-local once arrangements are placed correctly, and both sides of a join agree on placement because they route by the same function of the key. Testing: the backend gate now runs each of its 14 programs at 1, 2, 3 and 4 workers (3 to take the modulus path rather than the mask) and again over ProcessBinary channels, which serializes every exchanged container -- the multi-process path without the processes. `evaluate_with_workers` / `evaluate_with_config` are the entry points that makes that possible. Unit tests cover the partition (it is a partition; a key goes to one destination; routing does not depend on the batch, which is what makes a join correct; strided keys still spread) and the codec (round trips per shape family, truncation, and that a key column costs its payload). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The counting sort allocated its per-destination write cursor on every call, which is exactly the flat per-batch tax the other two index buffers are fields to avoid. The one per-batch allocation left is the hash column, which `corgi::hash` returns owned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A decoded container is four independently encoded columns, and nothing below this layer can say how many rows the message is supposed to have. The time column can: it is stored per row, so its length is the count the sender actually paid for, and keys, vals and diffs all have to match it. This is the DDIR half of the hardening in corgi's #13. That codec now guarantees a structurally sound Value, but not a small one -- the payload-free constructors declare rows without spending bytes, so a Unit names a trillion rows in sixteen bytes and nothing inside corgi can call that wrong. Here it is wrong, and cheap to say so. What the check does not reach is a claim nested under a List, where flattening legitimately multiplies and the row count stops bounding the element count. That is the sender's honesty, which is the boundary a cluster-internal exchange accepts anyway; `corgi::bytes::declared_rows` is there for a caller that wants a hard ceiling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
corgi #13 landed, so the rev moves from the unmerged `ddir-rem` branch to master. That is the last thing standing between this branch and a clean checkout: `corgi::bytes` is what `CorgiContainer`'s wire format is written in terms of, and until now it existed only behind a local `[patch]`. The rev also carries #11, #12, #14 and #15, since the old pin was a side branch off a master four commits older. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A container with no rows has no shape either. Every path that rebuilds one from rows infers its columns by scanning them, so `from_updates` on an empty vector returns the shape-erased default -- `Unit` keys and `Unit` vals. Hand that to the next op and `compile_projection` lowers `$1[0]` against a `Unit`, because a `Field` is legal in the abstract; `eval_graph` then meets a column that is not a product and panics with "Field: expected a product". Reaching it takes two steps, which is why it survived this long. First an op that empties a batch while KEEPING its shape -- the columnar filter path does that, since `gather` of an empty index list is still a typed zero-row column. Then an op whose term the lowering DECLINES, taking the row-wise path and rebuilding through `from_updates`. Only after those does a third op meet the erasure. Latent since the backend was written: one worker rarely empties a batch it was given. The key-hash exchange makes it routine, and it is a chain emptying ITSELF that does the damage -- a filter drops every row and hands the erased container to the very next op in the same `ops` slice -- so the guard belongs at the top of the loop, not before it. Skipping is the whole answer, not a shortcut: every `LinearOp` maps zero rows to zero rows, so the only thing the ops could contribute is the output shape, and the input's shape is already gone. No consumer reads it -- `CorgiChunker:: push_into` drops empty containers before `concat_blocks`, which is the one place shapes must agree, and `into_updates` and the distributor both short-circuit on zero rows. Found by running the AoC 2023 suite (33 parts, known answers) through the corgi backend at several worker counts: day03 part2 answered correctly at one worker and panicked at four. With the fix all 33 give identical results at 1, 3 and 4 workers; the 4 that still disagree are the pre-existing ones that suite's README already records, and they do not vary with worker count. tests/programs/empty_batch.ddp reproduces it in the gate, built from the failing day03 export rather than guessed at -- two earlier attempts passed with the fix reverted, because the filters I reached for compiled columnar and kept their shape. Without the guard this one wedges the gate rather than failing it, since a panicking timely worker leaves the others waiting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The corgi backend's
arrangeusedPipelineand assertedpeers() == 1, because a multi-worker run would have MIS-PLACED keys — silently wrong, not slow. This replaces that assert with the exchange it was waiting for, and the backend now renders on any number of workers, in any number of processes.The distributor
arrangeis the only exchange point in theBackendtrait, so substituting a pact forPipelineis the whole of multi-worker support: every other operator (linear,join,reduce,as_collection) is key-local once arrangements are placed correctly, and both sides of a join agree on placement because they route by the same function of the key.Timely's stock
DrainContainerDistributoris unusable here. ACorgiContainerhas no items to drain — it is four columns — and taking it apart row-wise would undo the representation before the data left the worker.CorgiDistributorpartitions the way a columnar engine does:corgi::hash— the same structural hashpresent_keyprepends as a key's identifier lane, so the distributor and the arrangement agree on what a key's identifier is;gathereach destination's rows into fresh contiguous columns — which is exactly what the wire format wants to write, one memcpy per leaf.Stable, so rows keep their order within a destination. A container whose rows all share a destination moves whole, with no gather at all. The index buffers are fields, not locals, so a batch costs no allocation beyond the hash column itself.
The wire format
corgi/bytes.rsisContainerBytesforCorgiContainer: four length words, then the key and value columns throughcorgi::bytes(landed as frankmcsherry/WIP#13) and the time and diff columns throughcolumnar'sStash— the encoderT: Columnarwas already chosen for. Every section is a whole number of words, so a receiver can install the bytes rather than relocate them.The contrast is the point: the row backend's
Vec<((Row, Row), T, R)>reaches the wire through bincode, walking everyValueof every row and emitting varints. This walks four columns and emits memcpys.The container decoder checks the four columns agree with the time column's length.
corgi::bytesguarantees a structurally soundValuebut not a small one — the payload-free constructors declare rows without spending bytes, so aUnitnames a trillion rows in sixteen bytes and nothing inside corgi can call that wrong. Here it is wrong, and this is the layer that knows.A latent bug the exchange made reachable
A container with no rows has no shape either: every path that rebuilds one from rows infers its columns by scanning them, so
from_updateson an empty vector returns the shape-erased default. Hand that to the next op andcompile_projectionlowers$1[0]against aUnit, because aFieldis legal in the abstract;eval_graphthen panics with "Field: expected a product".Reaching it takes two steps, which is why it survived: an op that empties a batch while KEEPING its shape (the columnar filter path —
gatherof an empty index list is still a typed zero-row column), and then an op whose term the lowering DECLINES, taking the row-wise path throughfrom_updates. One worker rarely empties a batch it was given; the key-hash exchange makes it routine.Fixed by skipping ops once a container is empty, at the top of the loop rather than before it — a chain empties itself, a filter dropping every row and handing the erased container to the very next op in the same slice. That is the whole answer rather than a shortcut: every
LinearOpmaps zero rows to zero rows, so the only thing the ops could contribute is the output shape, and no consumer reads an empty container's shape anyway.Testing
The gate.
tests/corgi_backend.rsruns each of its programs at 1, 2, 3 and 4 workers — 3 deliberately, so the modulus path runs rather than the mask and no input divides evenly — and again overProcessBinarychannels, which serializes every exchanged container. That last one is the multi-process path without the processes, so the wire format is exercised in-tree rather than only in a cluster.evaluate_with_workers/evaluate_with_configare the entry points that makes possible.The AoC suite. The 33 parts added in #847 have known answers, so they were run through the corgi backend at 1, 3 and 4 workers. This is what found the bug above: day03 part2 answered correctly at one worker and panicked at four. With the fix, all 33 give identical results at every worker count. Four still disagree with the oracle — day01 p1/p2 and day05 p1/p2 — but those are the pre-existing ones that suite's README already records, and they do not vary with worker count.
tests/programs/empty_batch.ddpreproduces the failure in the gate, built from the failing day03 export rather than guessed at: two earlier attempts passed with the fix reverted, because the filters I reached for compiled columnar and kept their shape.Real processes.
sccat n=20k with 200 rounds of incremental insertions and retractions gives the identical net count (25298) for vec at 1/2/4 workers, corgi at 1/2/4 workers, and corgi across two OS processes over TCP.Unit tests cover the partition (it is a partition; a key goes to one destination; routing does not depend on the batch, which is what makes a join correct; strided keys still spread, which routing by the raw key would not) and the container codec (round trips per shape family, truncation, and that a key column costs its payload).
Performance
M4, 10 cores. Load time, median of 3,
rounds = 0.2.8× at four workers on both programs, and corgi stays ahead of vec at every worker count. Both backends regress past four workers, which is this machine's P-core/E-core boundary rather than anything about the exchange.
Serialization costs +6.3%. Same program, four workers either way: 715 ms as one process with typed channels, 760 ms as two OS processes with the wire format plus TCP. vec pays +9.0% for bincode over the same step. corgi paying full serialization beats vec paying none.
The exchange itself is 1.2% — everything under
Exchange::pushin asamplyprofile ofsccat-w4is 35 of 2983 samples.Why 2.8× and not 4×, since that is the obvious next question. Total CPU rises 1.54× from one worker to four (1931 → 2983 samples), and 4 / 1.54 = 2.6. Of the 1052 added samples:
step_or_parkself time is 0 at one worker and 362 at four — timely's scheduler, a third of the loss — thenProxyReduceTactic::retire+178,MergeBatcher::seal+122,SmallVec::try_grow+91, which are per-batch fixed costs multiplying by worker count as each worker seals and retires and merges its own quarter. The exchange is +35. The lever for better scaling is batch granularity, not the shuffle.Two ideas measured and set aside
Ship each destination pre-sorted by key hash. The receiver's chunker sorts anyway, and has to — it is merging W senders' blocks, not one. Sorting at the sender adds a sort there and leaves the receiver's merge in place. The profile puts the receiver's whole ingest sort at 3.3%; there is no 20% sitting there.
Carry the hash lane across the exchange so the receiver's
present_keyneed not recompute it. Only bites compound keys —present_keyuses a primitive-integer key as it stands and hashes nothing, and scc, reach and stable are all scalar-keyed after lowering. It would need a flag on the container whose invariant is held by the dataflow's topology rather than by the type, which is fragile for a slice of an already-1.2% budget.Known cost
length_in_bytesandinto_bytesare separate calls on&self, and the time and diff columns must be built to be measured, so they are built twice — two linear passes overtimes, on the multi-process path only. The fix is the onecontainer.rsalready names: hold times columnar in the container instead of asVec<T>. Keys and values do not have this problem, since sizing aValuewalks its shape without touching a payload byte.🤖 Generated with Claude Code