fix(qdf): read a columnar frame into a slice whose element has its own codec - #183
Merged
Conversation
…ted twin Fails from sixteen elements under OptBalanced and OptCompression, at every size above: "qdf: type mismatch on decode". Reflect switches to the hybrid columnar container there, and a generated decoder accepts only the ONE column split baked into it at generation time, while reflect chooses its split by probing the data. The refusal itself is right — reading on would produce wrong values. What is missing is a way to fall back. Neither side is doing anything unusual: the producer is a plain library user with no generated code, the consumer only has the generated type. Service and GenService are defined types over one another, so this is the same value by construction rather than two similar ones. The reverse direction is asserted too and passes today — a generated producer's wire read through the plain type — so the pair pins both halves of the interop rather than only the broken one.
typeDesc.scopeToken and scopeFields were written for every Marshaler type at descriptor build and read nowhere. They are my own leftovers: the slice encoder used to bind a generated type's field scope, and when that binding moved inside EncodeQDF the reads went with it while the fields and their assignment survived the merge. Cost while it lasted: sixteen bytes on every typeDesc, plus an interface assertion and a method call on every Marshaler descriptor build. typeDesc 160 -> 144 bytes.
fillDesc returns early for a type implementing both Marshaler and Unmarshaler, leaving it with no fields, so buildColumnarPlan yields nil and a slice of such a type has no plan to read a columnar frame with. descBuild cannot supply one — it consults typeCache first and returns that same fieldless descriptor. structuralColumnarPlan builds the plan from a synthetic descriptor instead. It is not published to typeCache and the shared descriptor is untouched, which is what keeps this usable on decode without handing the slice encoder a plan it must not have. The test asserts the premise rather than assuming it, and was verified to fail when the fixture implements only one of the two interfaces: that type takes the structural switch and comes back with its fields populated. Nothing calls it yet.
… delta TestWideScratchSurvivesLargeColumns failed about one full-suite run in three, on main as well as on this branch, and its message blamed the widening scratch. A memory profile of a failing run says otherwise: widenI64 accounts for 2.6% of allocation and resetForReuse for all the rest. What the MemStats delta actually measured was the output-buffer hint. A pooled encoder coming back with an empty buffer pre-allocates cap(previous output) (encoder.go:524), so a hint left behind by a neighbouring test dominated the figure, and which encoder the pool handed out depended on GC timing. The verdict therefore inverted with the collector: GOGC=off failed five runs out of five and GOGC=1 passed three out of three — backwards for a test about scratch retention, and the reason isolation always passed. Now it asserts what putEnc promises, on the scratch itself: kept under the retention ceiling, dropped above it. Both arms were verified to fail when the corresponding half of putEnc is broken, and the second arm is what stops the first from passing with retention made unconditional. Twelve full runs clean, and five clean under GOGC=off where it previously failed every time. The product behaviour the old test stumbled over is left alone and is worth knowing separately: the buffer hint does not shrink for a smaller message while the encoder stays pooled, so one large message can size the next small one.
…n codec A wire written by the reflect encoder could not be decoded into a generated type from sixteen elements up, where the encoder switches to the hybrid columnar container: the slice descriptor carries no columnar plan for such an element, so the frame fell past the columnar dispatch into per-element decode and reported a type mismatch. Under OptCompression the same failure hid behind an entropy frame (0xbd, 0xd7, 0xef) whose payload is columnar once unwrapped. The frame is evidence about its own producer. A hand-written codec writes its own format and never emits a columnar frame, so meeting one means the structural encoder wrote it, and reading it structurally reproduces the value rather than guessing at it. The plan is built at most once per type, only after such a frame has actually been seen, and the shared descriptor is not touched — so encoding is unchanged and no type that builds a descriptor today begins to fail. Whether a type can reach the branch at all is decided once when the decoder closure is built, next to the existing elemDynamic / elemPF / elemHasMap hoists. Verified to fail again with the branch disabled: all eight rows of the matrix come back, so the test is not passing for an unrelated reason.
The fallback is only correct while it stays on the decode side, and a round-trip cannot tell the difference. TestDecodeFallbackLeavesEncodingAlone asserts the wire itself: re-encoding after a fallback decode must produce identical bytes, and a generated type must still refuse the columnar container its plain twin takes. It counts the rows where the plain twin really did produce a columnar frame and fails if none did, so the divergence check cannot go quiet. TestReflectWireIntoGeneratedTypeBoundary pinned the defect as expected behaviour, and did its job — it failed on this branch with "the boundary moved; if this is a fix, update the expectation". There is no boundary left to pin, and TestReflectWireDecodesIntoGeneratedType covers the same ground strictly better: six lengths against five, three option sets against one, and the decoded values compared rather than only the absence of an error. Keeping a weaker duplicate would be maintenance debt, so it is removed and the sibling that referred to it now says where the property lives. Verified: four modules green, race clean, the two-phase codegen CI job run locally (both phases), FuzzDecoder_NeverPanics 9,960,994 execs and FuzzRoundTrip_StringSlice 6,161,337 execs with no crashers. Columnar 38, ColumnIndex 1, Query 25, Select 12, Pushdown 1, Predicate 5, Batch 53, Skip 33, Hybrid 12 tests passing and none failing — counted rather than asserted clean, so an empty match cannot pass for coverage.
Benchmarked against the row-major wire for the same values rather than against
the error it replaces, since before this branch the columnar arm did not decode
at all and has no earlier number.
At 512 elements, decoded into the same type:
columnar (OptBalanced) 54,141 B 139.7us +/- 1%
row-major (OptSpeed) 246,278 B 130.1us +/- 1%
The wire is 4.5x smaller and the decode is 7.4% SLOWER. The design note predicted
the opposite — that reading one column at a time would also win on time — and that
claim is withdrawn rather than dropped. It changes nothing about doing the work:
the alternative to a slower decode here is no decode at all.
Bench gate for the branch, benchstat n=10 interleaved from a worktree at base
50a8c7b: decode geomean -0.01% with memory unchanged. The encode side moved
+0.36% geomean, and it is layout rather than logic — structuralColumnarPlan has
exactly one caller, inside the closure assigned to td.decode, so no encode path
can reach the new code; on a repeat run one PFor benchmark of six moved instead
of five, which is how alignment noise behaves and not how a logic change does.
The encoder's output bytes are pinned identical by
TestDecodeFallbackLeavesEncodingAlone regardless.
An element type the fallback cannot describe structurally was refused correctly
but not remembered, so every later decode of that slice rebuilt its whole field
list, allocated the descriptors and threw them away again — unbounded repeated
work on the one path that gains nothing from it.
Measured on a columnar wire decoded into an element with a channel field: 17
allocations per decode before, under 8 after. The test asserts the allocation
count rather than timing, because a walk that no longer happens cannot allocate,
and it was verified to fail with the refusal cache removed.
Re-verified after the change: four modules green, race clean, both phases of the
codegen job.
Two gaps in the earlier verification were closed rather than assumed:
- A self-referential element type does not exhaust the stack. structuralColumnarPlan
calls buildStructFields with a fresh buildCtx, bypassing descBuild's cycle
registration, so this needed evidence and not reasoning: it returns with a warm
cache and with a cold one.
- A slice of POINTERS to a struct is never written columnar by reflect (0xB0,
0xD3 at 16/64/512) and decodes into the generated pointer type today, so the
Kind()==Struct predicate leaves no gap behind it.
Bench, on the class that PAYS for the fallback and gains nothing — a slice of a
generated type whose wire is row-major, which the root package cannot cover
because it has no generated types: all sizes statistically indistinguishable,
bytes and allocation counts identical.
The noise floor was measured rather than assumed. The same binary benchmarked
against itself reports -5.66% (p=0.045) and -3.79% (p=0.005), geomean -3.28%, so
this harness fabricates significant-looking differences of three to six percent.
Every movement seen on this branch is smaller than that.
The recorded number said the columnar decode is 7.4% slower than row-major. That
was measured on the default scalar build and quoted without saying so.
Nearly all of that decode is bit-unpacking an alphabet-packed string column —
readStringColumnAlpha is 13.4% of a profiled run and bitUnpackU64LEFast 9.6% of
it — and internal/bitpack already carries hand-written amd64/arm64 assembly for
exactly that, behind the qdf_simd tag, off by default.
default (scalar) columnar 194.6us row-major 134.5us columnar loses 45%
-tags qdf_simd columnar 118.8us row-major 133.5us columnar wins 11%
-38.96% on the columnar arm (p=0.000, n=10), row-major unmoved — the control that
says the vector path touches only the unpacking the columnar form uses.
So the design note's original prediction holds where the assembly is compiled in
and fails where it is not, and both numbers now travel with their build. The
benchmark comment carries the same warning, since quoting one row without the
build is how the wrong verdict got recorded in the first place.
Root, bitpack and qdf-bench suites all pass under -tags qdf_simd; CI already
benchmarks with it. Whether it should be the default is a separate question — on
amd64 it also needs GOEXPERIMENT=simd — and is not decided here.
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.
A wire written by the reflect encoder could not be decoded into a generated type from sixteen elements up, where the encoder switches to the hybrid columnar container. Neither side was doing anything unusual: the producer is a library user with no generated code, the consumer has only the generated type.
The cause was not what was recorded
The standing note blamed the generated decoder's baked column split. That is wrong for this case — the refusal happens before any generated code runs.
decodeSlicedispatches a columnar frame only when the slice descriptor carries a plan, and for an element with a codec of its own that plan is nil. Two separate mechanisms produce the nil: a type implementing bothMarshalerandUnmarshaler(every generated type) returns early fromfillDescwith no fields, andbuildColumnarPlanyields nil for a fieldless descriptor; a type implementing only one is caught by an explicit guard. The failing test takes the first path.The fix
structuralColumnarPlanbuilds a plan from a synthetic descriptor.descBuildcannot supply one — it consultstypeCachefirst and returns the same fieldless descriptor. The synthetic one is never published and the shared descriptor is never modified.That restriction is load-bearing, not tidiness: giving the shared descriptor fields would also give it a columnar plan, and that plan feeds the slice encoder. Decode-only would not survive it.
Reading such a frame structurally is faithful rather than a guess. A hand-written codec writes its own format and never emits a columnar frame, so meeting one is evidence the structural encoder wrote it.
Cost
Whether a type can reach the branch is decided once when the decoder closure is built, next to the existing
elemDynamic/elemPF/elemHasMaphoists. The plan is built at most once per type, only after such a frame has actually been seen, and lives in the closure rather than intypeDesc— which grows by nothing.A measurement whose sign depends on the build
Wire is 54,141 B columnar against 246,278 B row-major — 4.5x smaller — either way. Time is not so simple:
-tags qdf_simdNearly all of the columnar decode is bit-unpacking an alphabet-packed string column —
readStringColumnAlphais 13.4% of a profiled run,bitUnpackU64LEFast9.6% of it — andinternal/bitpackalready carries hand-written amd64/arm64 assembly for that, behind a tag that is off by default. Enabling it is -38.96% on the columnar arm (p=0.000, n=10) with row-major unmoved, which is the control: the vector path touches only the unpacking the columnar form uses.An earlier revision of this description said flatly that the fallback is "7.4% slower". That was the scalar build, quoted without saying so. Both numbers now travel with their build, in the spec and in the benchmark comment.
None of it changes whether to do the work: the alternative to a slower decode here is no decode at all.
Whether
qdf_simdshould be the default is a separate question and is not decided here — on amd64 it also needsGOEXPERIMENT=simd. Root, bitpack and qdf-bench all pass under the tag, and CI already benchmarks with it.Verification
Encoding is pinned byte-for-byte: re-encoding after a fallback decode must produce identical bytes, and a generated type must still refuse the columnar container its plain twin takes. That test counts the rows where the plain twin really did produce a columnar frame and fails if none did.
Both new behaviours were verified by breaking them — with the branch disabled all eight rows of the failure matrix return; with the refusal cache removed the allocation assertion fires.
Four modules green, race clean, both phases of the codegen CI job run locally.
FuzzDecoder_NeverPanics9,960,994 execs andFuzzRoundTrip_StringSlice6,161,337 execs, no crashers. Columnar 38, ColumnIndex 1, Query 25, Select 12, Pushdown 1, Predicate 5, Batch 53, Skip 33, Hybrid 12 tests passing, none failing — counted, so an empty match cannot pass for coverage.Two gaps were closed with evidence rather than reasoning: a self-referential element type does not exhaust the stack (the fresh
buildCtxbypassesdescBuild's cycle registration, so this needed testing), and a slice of pointers is never written columnar by reflect, so theKind()==Structpredicate leaves no gap.Bench. Decode geomean -0.01%, memory unchanged, n=10 interleaved from a worktree at base. The noise floor was measured rather than assumed: the same binary against itself reports -5.66% (p=0.045) and -3.79% (p=0.005), geomean -3.28%, so this harness fabricates significant-looking differences of three to six percent and every movement on this branch is smaller than that.
Included, found on the way
A flaky test on main, root-caused.
TestWideScratchSurvivesLargeColumnsfailed about one full-suite run in three and blamed the widening scratch. A memory profile of a failing run putswidenI64at 2.6% of allocation andresetForReuseat all the rest: it was measuring the output-buffer hint, which a pooled encoder pre-allocates from the previous message, so a hint left by a neighbouring test dominated the figure. The tell is counter-intuitive — the verdict inverted with the collector,GOGC=offfailing 5/5 andGOGC=1passing 3/3. It now asserts whatputEncpromises directly on the scratch, both arms verified by breaking the matching half. Twelve full runs clean.Dead code.
typeDesc.scopeToken/scopeFieldswere written for every Marshaler type and read nowhere, left behind when the field-scope binding moved intoEncodeQDF.typeDesc160 -> 144 bytes.One inefficiency in this branch's own code. A refused element was re-walked on every decode: 17 allocations per decode, now under 8.
Unblocks
Columnar encoding for a slice of a generated type — the +62.6% wire gap at 512 elements — which needed this first and is a separate change with a much wider blast radius.
Left alone, worth knowing
The buffer hint does not shrink for a smaller message while the encoder stays pooled, so one large message can size the next small one until GC drops the encoder. Deliberate, and untouched here.