cabi: one cached layout node per type, not a walk per element per field (#261) - #263
Merged
Conversation
…ld (#261) `loadRecord` called `alignment` and `elemSize` per field per element, each an independent recursive walk; `loadVariant` called `maxCaseAlignment` on every element, walking every case into its record's fields; `despecialize` allocated a fresh record or variant on every call and sits at the top of `load`, `store`, `alignment`, `elemSize`, `lowerFlat` and `contains`; and `discriminantType` allocated a `PrimType` that three call sites wanted only to measure. None of it is data-dependent — layout is a pure function of (ValType, PtrType), and plan/loader.ts builds each type node once and never mutates it. Rather than memoize those four functions independently — which would still leave `loadRecord` doing ~8 map lookups and a float division per field per element to recover a constant integer offset — this caches one `Layout` node per (type, pointer width), carrying the despecialized type, alignment, size, the record's field offsets and the variant's discriminant width and payload offset. The record and variant loops become indexed offset reads. The offsets are exactly the intermediate values `elemSizeRecord`/`elemSizeVariant` already computed and discarded, retained rather than re-derived, so they cannot drift from the spec's arithmetic; the per-kind kernels stay the line-by-line mirror of definitions.py and remain the source of truth for size and alignment. Two WeakMaps, one per pointer width: layout is a function of both, and a single map keyed on the type alone returns i32 layouts to a memory64 instance, silently and with no corpus coverage. layout_cache_test.ts pins that specifically. Everything `despecialize` constructs is frozen. The memo hands every caller the same object where each used to get a fresh one, and an audit of today's call sites says nothing about tomorrow's; freezing costs once per type and lets the engine hold the invariant. The comment states which half that covers and which half rests on the loader's immutability instead. Also: `alignTo`'s float divide drops out of the two bounds assertions (`ptr % align === 0` is the same test for power-of-two alignments, now asserted in `mkLayout` rather than assumed); `matchCase` uses a memoized label->index map instead of building an array per variant stored; and three sites that synthesized a fresh `ValType` per call — the parameter spill tuple, the event payload words, `discriminantType`'s return — are hoisted, since under identity-keyed caching a per-call type is a guaranteed miss plus a wasted insert. Measured on the lane added in #262 (deno, n=10000, interleaved before/after): lift-ops 2948 -> 809 ns/element (3.6x), lower-ops 3063 -> 728 ns/element (4.2x). Flat shapes: send-sync unchanged, send/recv at 1200 B about 1.2x. Behavior is unchanged with one degenerate exception worth recording: for a nested empty record at a misaligned pointer, the assertion that fires is now "empty record" rather than "load misaligned", because the layout node computes size and alignment together. Both are AssertionError, both inputs are spec-invalid per definitions.py's `assert(s > 0)`, and neither is reachable from a valid plan. Gates: check, test-conventions (goldens byte-identical), test-runtime (696 passed), conformance (1475 commands, 0 failed, 0 stale xfails) — the corpus lane included, which #261's reporter could not run.
lannbot
enabled auto-merge
September 3, 2026 23:18
This was referenced Sep 3, 2026
lannbot
pushed a commit
that referenced
this pull request
Sep 4, 2026
… measure #263/#264/#265 moved the boundary numbers enough that the committed baseline now misleads: the compound-element rows read ~4-5x high, and the block recording them is still headed "pre-#261 optimization" with no "after" anywhere. Adds a 2026-09-04 block alongside the 2026-08-11 one rather than overwriting it — a dated baseline is a historical record, and overwriting it destroys the before/after that makes the numbers mean anything. The new block carries the compound-element table and nothing else, on purpose. This box cannot currently reproduce the calls-per-second table: `send immediate 0` on the node-jspi lane read 780,785/s, then 1,023,625/s, then 521,044/s across three runs whose code differed only by the changes under test. Committing that would be noise with a date on it, and the README's own framing — compare the same lane across commits on one box — is precisely the use it would break. What is known instead is stated as a delta from interleaved before/after pairs (medians of paired differences, reproduced across two passes): send-sync +27%/+32%, send +22%/+28%, recv +34%/+31%. The 2026-08-11 table stays the recorded absolute baseline, labelled as understating the current tree. Stream rows are untouched for the same reason and it is stated: stream-sink at 256 KiB spans 2,900-10,800 MB/s across four interleaved runs with no consistent sign, and none of the three PRs touch the stream<u8> bulk-copy path. Also: the calls-per-second and stream tables padded lane columns to 22 characters, narrower than the longest lane name, so their headers ran together — which is why the committed baseline block is unreadable in exactly that spot. Widened to 26, matching the compound table.
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.
Fixes the per-element/per-field layout recomputation reported in #261. The instrument for it landed in #262; this is what moves those rows.
The defect
loadRecordcalledalignmentandelemSizeper field, per element — each an independent recursive walk of that field's subtree — plusalignTo, a float divide.load()itself called both again for its two bounds assertions, so a field cost ~4 full type-tree walks, not 2.loadVariantcalledmaxCaseAlignmenton every element, walking every case into its record's fields.despecializeallocated a fresh record/variant on every call, and sits at the top ofload,store,alignment,elemSize,lowerFlatandcontains.discriminantTypeallocated aPrimTypethat three call sites wanted only in order to measure it.None of it is data-dependent. Layout is a pure function of
(ValType, PtrType), andplan/loader.tsbuilds each type node once and never mutates it — the same premiseembedder/values.ts'scheckNoCollisionsalready relies on.Why a layout node rather than four memos
#261 proposes memoizing
alignment/elemSize/maxCaseAlignment/despecialize. That is directionally right but leavesloadRecorddoing roughly eight map lookups plus a float division per field per element to recover what is a constant integer offset.This caches one
Layoutper(type, pointer width)carrying the despecialized type, alignment, size, the record's field offsets, and the variant's discriminant width and payload offset. The record and variant loops become indexed offset reads — noalignTo, noalignment, noelemSizeper field. It fixes lowering at the same time (store.tshad the identical defect), and it is the IR a compiled lift (#8) would consume, so it is not throwaway work.The offsets are exactly the intermediate values
elemSizeRecord/elemSizeVariantalready computed and discarded — retained, not re-derived, so they cannot drift from the spec's arithmetic. The per-kind kernels stay the line-by-line mirror ofdefinitions.pyand remain the source of truth for size and alignment.Correctness
layout_cache_test.tspins this — verified during review that a single-map implementation fails that test.load/storehave already asserted the base is aligned to the node's own alignment (≥ every field's, ≥maxCaseAlignment) and alignments are powers of two, so the base factors out of thealignTo. Review traced every path reaching the record/variant kernels — including via lists atptr + i*size, the spill tuple, andlift.ts/lower.ts— and the precondition holds unconditionally on all of them.despecializeconstructs is frozen. The memo hands every caller the same object where each used to get a fresh one; an audit of today's call sites says nothing about tomorrow's. The doc comment states which half the freeze covers and which half rests on the loader's immutability.ptr % align === 0replacesptr === alignTo(ptr, align)— the same test for power-of-two alignments, minus the float divide. The power-of-two premise is now asserted inmkLayoutrather than assumed.matchCaseuses a memoized label→index map instead of building an array per variant stored. Duplicate labels map to-1, preserving the old "exactly one match" reject set exactly; both assertion messages verbatim.ValTypeper call — the parameter spill tuple, the event payload words,discriminantType's return — are hoisted, since under identity-keyed caching a per-call type is a guaranteed miss plus a wasted insert.One degenerate behavior change, recorded rather than hidden: for a nested empty record at a misaligned pointer the assertion that fires is now
"empty record"rather than"load misaligned", because the layout node computes size and alignment together. Both areAssertionError, both inputs are spec-invalid perdefinitions.py'sassert(s > 0), neither reachable from a valid plan.Measured (#262's lane, deno, n=10000, interleaved before/after on one box)
lift-opslower-opsFlat shapes:
send-syncunchanged;send/recvat 1200 B about 1.2x (noisy, 1.15–1.5x across runs) — the async call paths were paying per-call despecialization too.Note for expectations:
plan/loader.tsbuilds a freshValTypeper syntactic occurrence with no interning, so the caches key on occurrences. The entire win comes from reuse within one occurrence — per element, per field — which is exactly what #261 targeted.Gates
just check,just test-conventions(32 passed, goldens byte-identical — zero updates),just test-runtime(696 passed),just conformance(1475 commands, 0 failed, 0 stale xfails). The corpus lane ran — #261's reporter flagged it as unverified because thethird_party/component-modelsubmodule was not checked out in their tree.No contract change, no published-surface change (
cabi/mod.tsis not inruntime/deno.json's exports), no version bump.Reviewed
Independently reviewed against
definitions.py§§Despecialization/Alignment/Element Size/Loading/Storing,docs/architecture.md§1/§7, andAGENTS.md"Against accretion". Review returned one blocking finding —discriminantSizehad forkeddiscriminant_type's table, so flattening and layout could drift on the discriminant width — now fixed by derivingdiscriminantTypefromdiscriminantSize, one table. Plus: redundantLayoutparameters replaced by the derived quantities, and 15 tautological assertions removed from the test in favour of literal spec-table expectations (falsifiability confirmed by sabotagingcomputeLayout'sstringarm and watching the test fail).