repr: decode rows through a predicted per-column class - #38400
Conversation
5ae918c to
0f2b9e7
Compare
QA LLM Review1. MEDIUM -- A first row that classifies no column disables the prediction for the life of the
|
`read_datum` dispatches over roughly thirty tag classes per datum. That body is too large for a caller to inline, so decoding a row costs a function call per datum returning a 48-byte `Datum` through a hidden pointer. Selecting the arm from the column's class rather than from the tag shrinks the match to nine arms, which does inline, and that is where the time goes. The class is a prediction and is never trusted. Every arm checks that the tag really belongs to the predicted class and returns `None` otherwise, so `Prediction::decode` falls back to `read_datum` for that datum and learns the column from its tag. A wrong prediction costs throughput and never correctness, which is what lets `DatumVec` learn the prediction from the rows it decodes instead of being handed column types. That matters because the compute plan carries no column types: `BuildDesc` is an id and a plan, and `ReprRelationType` is only available at the dataflow boundary. `DatumVec` gains the prediction as a field, so `borrow_with` improves without any caller changing. One instance decodes rows from one collection, so the schema is stable and the prediction settles after the first row that is not null in every column. Five details are load-bearing, each of them measured. `Unknown` and `Other` are separate classes. `Unknown` always misses so a column learns on its first datum, while `Other` is terminal so a column of an uncovered type stops re-classifying on every row. A `Null` tag is accepted by every class but teaches nothing, so a column whose first value is null stays unclassified rather than being mispredicted, and tries again on the next row. A relation every column of which has settled on `Other` uses the general decoder directly, guarded by a check that the row's arity has not changed. Predicting such a relation cannot pay for itself, and the per-datum bookkeeping it needs measured 9% slower than not predicting at all. The condition is "every column has settled on `Other`" and not "no column has a fast arm", because the latter also holds while columns are still unclassified, which would stop the learning loop from ever running again and leave a relation whose fast columns are null in its first row decoding generally for the life of the decoder. Whether every column has settled is rescanned only when a class changed or truncation dropped one, which keeps the scan off the settled path. Rescanning unconditionally measured 2 to 3 percent slower on the integer shapes. There is no arm for `Numeric`, even though decimals are common, because such an arm can only defer to the general coefficient decode. Adding one measured 24% slower than not covering the type at all, since it puts a dispatch and a tag compare in front of identical work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0f2b9e7 to
2839358
Compare
|
All three findings are correct. Fixed in 2839358. 1. First row that classifies no columnReproduced before fixing. The mechanism is that Both of your cases now exist as tests, and both failed on the previous commit with exactly the states you predicted: Took the One correction to the claim that it costs nothing. Rescanning on every row measured 2 to 3 percent slower on the integer shapes, largest where throughput is highest, which is consistent with one extra compare per datum:
Worth flagging on measurement: my first run of this comparison showed a twenty percent swing on the integer shapes, including on 2. Every test passes if the fast path never firesCorrect, and finding 1 is the proof: the decoder was inert for those two shapes and nothing went red. Added 3.
|
Decoding a
Rowinto[Datum]costs about twice what it needs to, and the cause is the shape of the dispatch.read_datummatches over roughly thirty tag classes per datum, which makes its body too large for a caller to inline, so every datum pays a function call returning a 48-byteDatumthrough a hidden pointer. Selecting the arm from the column's class rather than from its tag shrinks the match to nine arms, which does inline.DatumVeclearns the class of each column from the rows it decodes and keeps it as a field, soborrow_withimproves without any caller changing.Why a prediction rather than the column types
The class is never trusted. Every arm checks that the tag really belongs to the predicted class and returns
Noneotherwise, soPrediction::decodefalls back toread_datumfor that datum and learns the column from its tag. A wrong prediction costs throughput and never correctness.That is what makes learning viable, and learning is what makes the change small. Passing types down instead would need them to exist at the call sites, and they do not:
BuildDescis an id and a plan, andsrc/compute-types/src/plan.rscarries no column types at all.ReprRelationTypeis available at the dataflow boundary, onindex_exportsandindex_imports.on_type, but not per plan node, so threading types through the physical plan would be a protocol change. Measuring the checked form against a form that is simply told the types put the check at 0 to 1 percent on integer columns and 8 to 10 percent on mixed ones, so the protocol change would buy very little.Measurements
cargo bench -p mz-repr --bench predict, throughput in Melem/s, batches of 1024 rows pinned to one core.generalisborrow_with_general, which decodes every datum throughread_datum;predictedisborrow_with.The last row is the shape with nothing to gain, and it is there to show the fallback does not cost anything; it sits at parity. Run to run variance on this machine is about three percent, so these come from a longer measurement window after a first run showed a twenty percent swing on the integer shapes.
The decoder in isolation reaches 2.0x to 2.5x. The rest is
borrow_withitself: the per-rowmem::takeand therepurpose_allocationinDatumVecBorrow::dropcost about 23 percent at arity 8, independently measured, and that share grows as the decode gets cheaper. Worth a separate change.Three results that shaped the design and are worth recording, since each looked promising and was not:
Datumis 48 bytes because ofRange, notNumeric. ShrinkingDatumwas the original hypothesis and it buys nothing.Details worth a reviewer's attention
Five things are load-bearing, and each was measured rather than assumed.
UnknownandOtherare separate classes and both are needed.Unknownalways misses, so a column learns on its first datum; with only one variant the first row would learn nothing.Othernever misses, so a column of a type no fast arm covers settles instead of paying a classification on every row.A
Nulltag is accepted by every class, since any column can be nullable, butclassifyreturnsNonefor it. A column whose first value is null therefore stays unclassified and tries again on the next row, rather than being mispredicted by the null.A relation every column of which has settled on
Otheruses the general decoder directly, guarded by a check that the row's arity has not changed. Predicting such a relation cannot pay for itself, and the per-datum bookkeeping measured 9 percent slower than not predicting at all. The condition is "every column has settled onOther" and deliberately not "no column has a fast arm": the latter also holds while columns are still unclassified, which would stop the learning loop from running again and leave a relation whose fast columns happen to be null in its first row decoding generally for the life of the decoder.Whether every column has settled is rescanned only when a class changed or truncation dropped one, which keeps the scan off the settled path. Rescanning on every row measured 2 to 3 percent slower on the integer shapes.
There is no arm for
Numeric, even though decimals are common. Such an arm can only defer to the general coefficient decode, so it puts a dispatch and a tag compare in front of identical work; it measured 24 percent slower than not covering the type. Removing it also made every other shape faster, since the match got smaller.decodealso truncates the class vector to the row's arity, so a change in arity self-corrects.borrow_with_generalis retained only so the benchmark can measure against the path this replaces. It should go once the change is settled.Tests
Every datum decodes correctly whether or not the prediction is right, so tests that assert only on decoded datums cannot tell a working fast path from an inert one.
Predictiontherefore carries#[cfg(test)]counters of hits, misses and rows that skipped prediction, and the tests assert on them.src/repr/src/row/predict.rsadds:matches_general_decoder, a proptest that decodes generated rows through the learned sequence and through every fixed class including deliberately wrong ones, asserting equality withread_datumthroughout.prediction_settles, which asserts that after the first row every datum is a hit and no row falls back wholesale.first_row_teaching_nothing_still_learnsanduncovered_column_does_not_mask_a_learnable_one, covering a first row that classifies nothing, either because the only column is null or because the one column with a fast arm is.null_teaches_nothingandarity_change_settles.src/repr/benches/predict.rsis new and covers integer, mixed and numeric-only shapes, the last being the no-gain case.Not in this change
No feature flag.
mz_reprhas no dyncfg access, so gating needs either an atomic static set at startup or gating at the call sites. Correctness does not depend on a flag, because the per-datum check makes a wrong prediction a throughput question only, but this should probably not go to production ungated. Happy to add whichever form reviewers prefer.The share of clusterd on-CPU time spent decoding rows has not been measured, so what this is worth end to end is unknown.
Part of: CPU-223
🤖 Generated with Claude Code