cabi: the internal variant value carries kind, not a computed key (#261) - #270
Merged
Conversation
…261) A lifted variant was a single-key object keyed by the case label, `{"set-attribute": payload}`. Three costs, all per variant per element: - a computed-key object literal, which is not V8's fast literal path — each distinct label yields its own hidden class, so with a 16-case variant every reader of that value ran megamorphic; - an `Object.keys()` array allocation at each end (`single()` in the embedder, `matchCase()` in the CABI) to read exactly one key; - and the `[label, payload]` tuple `single()` returned. It is now a fixed-shape `{kind, value}` — one hidden class for every variant value in the program, and both `Object.keys` calls deleted. The whole despecialized family moves together: plain variant, enum, option, and result (whose error case is still spelled "error" internally, per definitions.py). Records, tuples and flags are unchanged. `value` is always present, `null` for a payload-free case. Matching the host layer exactly by omitting it instead was prototyped and measured SLOWER — about five points on lift, and much noisier — because the producer site then emits two shapes rather than one. One shape everywhere beats exact convergence. The property names deliberately match the host variant shape. That is the direction contracts/embedder-api.md already anticipated ("converging the interpreter itself is a perf-track concern"), and it is a trap as well as a win: an internal value and a host value can now be structurally identical and mean different things. The four residual differences — result's "error" vs "err", enum as an object vs a bare string, option's outermost unwrapping, and the payload-free spelling — are enumerated in contracts/embedder-api.md §"Implementation strategy" and cited from the code. Sites that read a variant structurally without consulting the type do not fail loudly under the new shape; they return `undefined` and carry on, which is how two of them were found (only because a golden broke). The audit is closed against HEAD rather than sampled, and independently re-run by review. contracts/descriptor-ir.md now says what it should have said before: the raw boundary mirrors definitions.py's SEMANTICS, and its representation is ours to choose where measurement justifies it (architecture.md §1). The old prose claimed we produce the reference's shapes, which was the thing this change makes false. Fixes a latent bug found on the way: tools/smoke-c0's `unwrapOk` tested `"err" in r`, a spelling the raw boundary never produces, so its error branch was dead and an err result silently became `undefined`. Measured on the #262 lane, deno, n=10000, interleaved before/after across two independent passes (20 pairs total): lower-ops ~12-13%, positive in 19 of 20 pairs. lift-ops is positive in 16 of 20 but poorly resolved on this box — the two passes' medians are +16.6% and +4.5%, spanning -13% to +28% pair to pair. Reported as measured rather than picking the flattering half; see bench/boundary/README.md on why this box cannot do better. Ruled out by measurement, not argument: a case INDEX instead of the label (worth 1-2.5 points, and it would force WIT types through the conformance oracle, which is designed not to need them); and skipping `toHost` entirely where it is the identity function (worth ~6% on lift, and lost outright to this plainer shape). Gates: check, test-conventions (goldens byte-identical), test-runtime (698 passed), conformance (1475 commands, 0 failed, 0 stale xfails), smoke-c0. Two tests added for paths no gate covered, each verified to fail when its source line is reverted.
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.
Follows #263/#264/#265. This is the "structural, not a hoist" item I deferred out of those — the CABI building
{[label]: value}only fortoHostto take apart on the next line.The defect
A lifted variant was a single-key object keyed by the case label,
{"set-attribute": payload}. Per variant, per element:Object.keys()array allocation at each end —single()in the embedder,matchCase()in the CABI — each to read exactly one key.[label, payload]tuplesingle()returned.Now a fixed-shape
{kind, value}: one hidden class for every variant value in the program, and bothObject.keyscalls deleted along withsingle()itself. The whole despecialized family moves together — plainvariant,enum,option,result(error case still spelled"error"internally, perdefinitions.py). Records, tuples and flags are untouched.Two decisions made by measurement, not argument
Three throwaway prototypes preceded this, and two plausible-looking options lost:
harness/src/value-mapping.ts, the conformance oracle, which is deliberately designed not to need them (schema.tsgives it a case label and no case list). Trading correctness risk in the component that decides whether everything else is right, for two points. No.toHostwhere it is the identity function — a memoized per-type predicate, ~40 lines — bought ~6% on lift and still lost outright to the plainer shape below.valuealways present,nullfor payload-free. Matching the host layer exactly by omitting it measured ~5 points slower and much noisier, because the producer site then emits two hidden classes instead of one. One shape everywhere beats exact convergence.The trap this creates, and where it's written down
The property names deliberately match the host variant shape — the direction
contracts/embedder-api.mdalready anticipated ("converging the interpreter itself is a perf-track concern"). The cost is that an internal value and a host value can now be structurally identical and mean different things. The four residual differences (result's"error"vs"err",enumas an object vs a bare string,option's outermost unwrapping, and the payload-free spelling) are enumerated in that document's §"Implementation strategy" and cited from the code.Sites that read a variant structurally without consulting the type —
"error" in v,v["ok"]— don't fail loudly under the new shape. They returnundefinedand continue. That's how two of them were found during prototyping: only because a conventions golden broke. The audit was therefore closed againstHEADrather than sampled, and independently re-run by review.contracts/descriptor-ir.mdalso now says what it should have said before: the raw boundary mirrorsdefinitions.py's semantics, and its representation is ours to choose where measurement justifies it (architecture.md§1). The old prose claimed we produce the reference's shapes — the thing this change makes false.A latent bug fixed on the way
tools/smoke-c0'sunwrapOktested"err" in r— a spelling the raw boundary has never produced. Its error branch was dead code, and an err result silently becameundefined.Measured
#262's lane, deno, n=10000, interleaved before/after across two independent passes, 20 pairs total:
lower-ops~12-13%, positive in 19 of 20 pairs.lift-opspositive in 16 of 20, but poorly resolved on this box: the two passes' medians are +16.6% and +4.5%, spanning −13% to +28% pair to pair.Reported as measured rather than by picking the flattering half —
bench/boundary/README.mddocuments why this box can't do better, and this PR doesn't refresh the committed baseline for the same reason.Gates
just check,just test-conventions(32 passed, goldens byte-identical),just test-runtime(698 passed),just conformance(1475 commands, 0 failed, 0 stale xfails),just smoke-c0,just version-guard-local.Two tests added for paths no gate covered —
#buildSyncForm's result-ok branch andtoHost's record-field-of-option-type — each verified to fail when its source line is reverted, because a test that passes either way is decoration.Contract change (internal shape only). The host ABI is unchanged, which the byte-identical goldens are the evidence for: no
breaking/protocol, no version bump.