cabi: memoize the flattened param/result count, not per call (#261) - #265
Merged
Conversation
`liftFlatValues` and `lowerFlatValues` called `flattenTypes(ts, cx.opts)` on every boundary crossing — a full recursive walk of every parameter type, allocating an array at each level via `flatMap` — and used the result for exactly one thing: `flatTypes.length > maxFlat`. They never look at the elements. Flattening reaches `opts` only through `requireMemory(opts).ptrType()`: string and unbounded-list read the pointer width, every other arm is opts-free or recurses structurally. So the count is a pure function of (ts, ptrType), and `ts` is always a plan-owned `ft.params`/`ft.results` array — the same stability `spillTupleType` already relies on. Caches the COUNT rather than the flattened array. A cached array would need freezing plus a lossy readonly-to-mutable cast at every read site; a number has no aliasing hazard at all, and nothing on the per-call path wanted more than the number. `flattenTypes` itself is untouched, so a reader auditing "did this change flattening semantics" sees a no-op diff around it. `flattenFunctype` still calls it directly and uncached — that path runs once per function at instantiate time, not once per call. The null-memory path stays uncached: `requireMemory` throws there, and that throw must surface on every call rather than only the first. This is not on #261's checklist; it is the same defect one level up, found while working the per-element paths — and it is the largest win of the series, because it is per CALL and the calls-per-second lanes are what #17's jco comparison tracks. Measured on bench/boundary (deno, immediate, 50000 iters, size 0, interleaved before/after, medians of paired differences): send-sync +27%, send +22%, recv +34% calls/sec. Independently reproduced by the orchestrator at +32%/+28%/+31%. Gates: check, test-conventions (goldens byte-identical), test-runtime (696 passed), conformance (0 failed, 0 stale xfails).
lannbot
enabled auto-merge
September 3, 2026 23:33
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
2 participants
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.
Not on #261's checklist — this is the same defect one level up, found while working the per-element paths. It is the largest win of the series, because it is per call rather than per element, and the calls-per-second lanes are what #17's jco comparison tracks.
The defect
liftFlatValuesandlowerFlatValuescalledflattenTypes(ts, cx.opts)on every boundary crossing — a full recursive walk of every parameter type, allocating an array at each level viaflatMap— and used the result for exactly one thing:They never look at the elements.
flattenFunctype, the caller that does want the types, runs at instantiate time only (exec/boundary.ts:1378,2118, once per function, to check the computed flat type against the shim'scoreType).Why the count is cacheable
Every arm of
flattenTypereachesoptsonly throughrequireMemory(opts).ptrType()—stringand unboundedlistread the pointer width, everything else is opts-free or recurses structurally. Audited arm by arm. So the count is a pure function of(ts, ptrType), andtsis always a plan-ownedft.params/ft.resultsarray, the same stabilityspillTupleTypealready relies on.Why a count and not the array
The first version of this cached the flattened array, which meant
Object.freeze(...) as CoreType[]— a frozen array cast to a mutable type. That compiles fine for a caller that mutates and throws at runtime, with only an audit standing in between. Returningreadonly CoreType[]honestly would have rippled intoCoreFuncType.paramsand out into the descriptor-IR surface.Caching the number collapses the whole soundness argument to "the count is a pure function of
(ts, ptrType)". A number cannot be aliased or mutated.flattenTypesis left byte-identical, so anyone auditing "did this change flattening semantics" sees a no-op diff around it.The null-memory path stays uncached:
requireMemorythrows there, and that throw must surface on every call rather than only the first.Measured
bench/boundary, deno,immediate, 50000 iters, size 0 (payload would only dilute a per-call effect). Interleaved before/after, medians of paired differences:send-syncsendrecvIndependently reproduced by a second measurement pass at +32% / +28% / +31%, 8 of 9 pairs positive with one contention outlier.
Gates
just check,just test-conventions(32 passed, goldens byte-identical),just test-runtime(696 passed),just conformance(0 failed, 0 stale xfails). No contract change, no published-surface change, no version bump.