perf(core): codegen bindColumns runners — beat bitECS, no post-growth penalty#82
Conversation
… penalty
bindColumns is the SoA fast path, but the interpreted approach invoked
ONE user factory per archetype: the moment it produced a second runner
(a 2nd matched archetype, or a re-invoke after column growth) V8 saw the
factory making multiple closures and disabled specialization for all of
them — ~1.5 ns/entity, which LOSES to bitECS (~1.4). It only won on a
single pre-sized archetype.
Each archetype's runner is now CODEGEN'D: the factory is re-evaluated
into a DISTINCT function via new Function('return (' + factory.toString()
+ ')')(), so its runner is the singleton of its own freshly-minted
factory → specialized → ~1.0 ns/entity (~0.7x bitECS), through growth and
across archetypes, with NO pre-sizing. (Spike: 0.97 steady, 0.97 after
re-bind vs the interpreted 1.5.)
Safety — codegen can never change results:
- eval availability probed once; under strict CSP / a locked sandbox the
interpreted factory call is used (the runs-everywhere promise holds).
- the factory must be SELF-CONTAINED (close over nothing — the recompiled
copy sees only globals); per-frame inputs arrive via the runner's new
ctx argument, hoisted out of the loop.
- a PRE-FLIGHT runs a recompiled runner and the interpreted runner over an
identical 1-row scratch clone and compares (Object.is, so identical NaN
writes match); codegen is used only on a match. A miscompile, an illegal
outer-scope closure (ReferenceError), or any divergence → interpreted.
Codegen is faithful by construction (a recompile of the same source), so
the only divergences are row-invariant closures — caught at row 0.
Security: the generated source is the caller's OWN factory.toString(),
never an interpolated external string — no injection surface.
Breaking (pre-launch): the runner gains a ctx arg —
factory: (views, meta) => (ctx) => void, run(ctx). Deps flow through ctx
instead of an outer closure (which codegen can't capture). Zero-arg
runners still typecheck (void ctx).
Adversarial review confirmed no silent-corruption path (the 1-row
pre-flight worry is a misframing — codegen ≡ interpreted by construction;
a user's own stride bug reproduces identically, never diverges). Fixed
its findings: Object.is for NaN, dead-param cleanup, stale README/docs
prose. The property suite now drives a self-contained factory so the
codegen path is its primary correctness gate (byte-identical to .each
under random spawn/despawn/grow).
Greptile SummaryThis PR introduces a codegen layer for
Confidence Score: 4/5Safe to merge; the codegen mechanism is guarded at every exit — CSP probe, pre-flight equality check, and try/catch fallback — so no code path can silently produce wrong results. The core logic is sound: meta.count remains a live getter (unchanged), the pre-flight correctly tests structural closure equivalence against scratch data, and the fallback chain is airtight. The two findings are about documentation accuracy — the 'invokes the runner once' NOTE fires twice, and the eagerly-created interpreted runner with real views is not mentioned in the public contract. Neither affects runtime correctness. packages/core/src/query/codegen.ts — the two NOTE-level doc inaccuracies are in this file. Important Files Changed
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/core/src/query/codegen.ts:76-84
**Interpreted runner always eagerly created even on the codegen path**
`factory(views, meta)` (line 76) is called with the real production views unconditionally, creating an interpreted runner even when codegen will succeed and the runner will be discarded. On the happy codegen path the factory is therefore called twice with real data (once for `interpreted`, once for the recompiled copy on line 83) plus twice more with scratch arrays inside `preflightMatches`. For a factory that is a pure closure factory this is harmless, but a factory with construction-time side effects (e.g. an initialisation log, a registration call) will fire those effects even when the codegen runner is ultimately used. The public-facing comments describe runner-invocation side effects but do not document that the factory-creation code also runs on a discard path.
### Issue 2 of 2
packages/core/src/query/codegen.ts:91-98
**Pre-flight NOTE says "invokes the user's runner once" but it fires twice**
The NOTE on line 95 reads "this INVOKES the user's runner once over the scratch," but `preflightMatches` actually invokes it twice — once for the recompiled version (`scratchA`, line 110) and once for the interpreted version (`scratchB`, line 111). A runner with observable side effects (the exact scenario the NOTE is warning about) would see those effects fire on both runs. The wording should say "twice" (or "once per version") to avoid a subtle expectation mismatch for users debugging unexpected pre-flight behaviour.
Reviews (1): Last reviewed commit: "perf(core): codegen bindColumns runners ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const interpreted = factory(views, meta) | ||
| if (!CODEGEN_AVAILABLE) return interpreted | ||
| try { | ||
| // The real runner is the singleton of its OWN recompiled factory (specialized). The pre-flight | ||
| // validates a SEPARATELY-recompiled runner over scratch — same source, so faithful ⇒ the real | ||
| // runner is faithful too. (Reusing one recompiled factory for both would make it produce two | ||
| // closures and forfeit specialization — the very penalty codegen exists to avoid.) | ||
| const codegen = recompile(factory)(views, meta) | ||
| if (preflightMatches(factory, views, strides)) return codegen |
There was a problem hiding this comment.
factory(views, meta) (line 76) is called with the real production views unconditionally, creating an interpreted runner even when codegen will succeed and the runner will be discarded. On the happy codegen path the factory is therefore called twice with real data (once for interpreted, once for the recompiled copy on line 83) plus twice more with scratch arrays inside preflightMatches. For a factory that is a pure closure factory this is harmless, but a factory with construction-time side effects (e.g. an initialisation log, a registration call) will fire those effects even when the codegen runner is ultimately used. The public-facing comments describe runner-invocation side effects but do not document that the factory-creation code also runs on a discard path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/query/codegen.ts
Line: 76-84
Comment:
**Interpreted runner always eagerly created even on the codegen path**
`factory(views, meta)` (line 76) is called with the real production views unconditionally, creating an interpreted runner even when codegen will succeed and the runner will be discarded. On the happy codegen path the factory is therefore called twice with real data (once for `interpreted`, once for the recompiled copy on line 83) plus twice more with scratch arrays inside `preflightMatches`. For a factory that is a pure closure factory this is harmless, but a factory with construction-time side effects (e.g. an initialisation log, a registration call) will fire those effects even when the codegen runner is ultimately used. The public-facing comments describe runner-invocation side effects but do not document that the factory-creation code also runs on a discard path.
How can I resolve this? If you propose a fix, please make it concise.
| /** | ||
| * Run a recompiled runner and a fresh interpreted runner over IDENTICAL 1-row scratch clones of the | ||
| * columns with the probe ctx, and compare. Equal ⇒ a recompile of this factory is faithful (and the | ||
| * real runner, recompiled from the same source, is therefore faithful too). The real columns are | ||
| * never touched. NOTE: this INVOKES the user's runner once over the scratch — a runner with effects | ||
| * beyond its views (a global write, a ctx method call) fires/throws here; the contract is a pure SoA | ||
| * loop reading values off ctx. A throw (e.g. an illegal outer-scope closure → ReferenceError) counts | ||
| * as a mismatch and falls back to interpreted. |
There was a problem hiding this comment.
The NOTE on line 95 reads "this INVOKES the user's runner once over the scratch," but preflightMatches actually invokes it twice — once for the recompiled version (scratchA, line 110) and once for the interpreted version (scratchB, line 111). A runner with observable side effects (the exact scenario the NOTE is warning about) would see those effects fire on both runs. The wording should say "twice" (or "once per version") to avoid a subtle expectation mismatch for users debugging unexpected pre-flight behaviour.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/query/codegen.ts
Line: 91-98
Comment:
**Pre-flight NOTE says "invokes the user's runner once" but it fires twice**
The NOTE on line 95 reads "this INVOKES the user's runner once over the scratch," but `preflightMatches` actually invokes it twice — once for the recompiled version (`scratchA`, line 110) and once for the interpreted version (`scratchB`, line 111). A runner with observable side effects (the exact scenario the NOTE is warning about) would see those effects fire on both runs. The wording should say "twice" (or "once per version") to avoid a subtle expectation mismatch for users debugging unexpected pre-flight behaviour.
How can I resolve this? If you propose a fix, please make it concise.1 participant
What
bindColumns(the pinned-columns SoA fast path) now codegens its per-archetype runners, so it beats bitECS robustly — not just on a single pre-sized archetype.The problem
The interpreted approach invoked ONE user factory per archetype. The moment it produced a 2nd runner — a 2nd matched archetype, or a re-invoke after column growth — V8 saw the factory making multiple closures and disabled specialization for all of them: ~1.5 ns/entity, which loses to bitECS (~1.4). It only won on a single pre-sized archetype (hence the old "pre-size before binding" caveat).
The fix
Each archetype's runner is recompiled into a distinct function —
new Function('return (' + factory.toString() + ')')()— so its runner is the singleton of its own freshly-minted factory → specialized → ~1.0 ns/entity (~0.7× bitECS), through growth and across archetypes, no pre-sizing.Safety — codegen can never change results
ctxarg (hoisted out of the loop).Object.is, so NaN writes match); codegen is used only on a match — a miscompile / illegal closure / any divergence → interpreted. Codegen is faithful by construction (recompile of the same source), so the only divergences are row-invariant closures, caught at row 0.factory.toString(), never an interpolated external string.Breaking (pre-launch)
The runner gains a
ctxarg:factory: (views, meta) => (ctx) => void,run(ctx). Deps flow throughctx(which codegen can capture) instead of an outer closure (which it can't). Zero-arg runners still typecheck (void ctx).Review
Independent adversarial review: no critical/major. Confirmed no silent-corruption path — the 1-row pre-flight worry is a misframing (codegen ≡ interpreted by construction; a user's own stride bug reproduces identically, never diverges). Fixed its 4 minors:
Object.isfor NaN, dead-param cleanup, stale README/_perf-tables prose.Verification
pnpm test→ 164 files / 1252 green (new codegen-pinned.test: CSP-availability, self-contained-codegen-through-growth, closure-fallback, pre-flight equality, multi-archetype; the property suite now drives a self-contained factory so codegen is its primary correctness gate — byte-identical to.eachunder random spawn/despawn/grow).typecheck:tests·typecheck:extras·docs:check.First of a 4-PR program to close the bitECS gaps (codegen
.each+ tracked-write, bundle budget) + a CI bench regression lane. Bench numbers held (load-gated; bindColumns 1.02 ns/e, no regression).