Refactor signal bumping and array mutator wrapping for efficiency#80
Open
scottmessinger wants to merge 9 commits into
Open
Refactor signal bumping and array mutator wrapping for efficiency#80scottmessinger wants to merge 9 commits into
scottmessinger wants to merge 9 commits into
Conversation
Replace per-array $MUTATORS cache + per-array wrapper closures with a single module-level table of nine wrappers (one per mutator name). Each wrapper forwards via `this`, so it needs no per-array closure capture. Eliminates: - One Object.create(null) cache per reactive array - Up to nine wrapper closures per array (one per mutator actually used) - One Object.defineProperty($MUTATORS) call per array - The getMutatorCache() helper - The $MUTATORS symbol A shared wrapper also helps V8: every `arr.push` site sees the *same* function reference, keeping inline caches monomorphic instead of going megamorphic across many arrays. Also: - Replace `as any` casts in collections.ts with the existing `ReactiveTagged` interface (regression of #76's type-soundness sweep). - Revert accidental package.json indent change (2-space → 4-space) in packages/js-krauset.
Four small allocation/lookup wins outside the proxy `get` handler (which the notes/failed-approaches/ files document as untouchable for V8 reasons): 1. write.ts setProperty/deleteProperty: look up `nodes` (target[$NODE]) once per call instead of up to four times. The previous code went through bumpVersion() + getNodesIfExist() + bumpSignals() + bumpOwnKeysSignal() — each of those did its own getNodesIfExist(target). Inlining the per-key / length / ownKeys signal updates into a single `if (nodes)` block keeps the hot write path on one node-bag reference. Behavior on non-extensible targets is preserved (getNodes still returns a transient bag whose slots are empty, so the inlined lookups no-op the same way the old guarded re-reads did — covered by store.test.ts "should not throw when deleting a key from a non-extensible target"). 2. for.ts swap effect: mutate `prevRawRef.current` in place instead of reallocating with `[...raw]` on every effect run. Saves one N-element array allocation per swap-effect tick. 3. for.ts swap effect: replace `changed: Array<number>` (allocated every tick to hold at most 2 indices) with two scalars + a counter. Same short-circuit semantics, no allocation. 4. core.ts: consolidate the two parallel `let BUMP = 0` counters from write.ts and collections.ts into a single `nextBump()` in core.ts. Pure cleanup — V8 trivially inlines the one-line monomorphic function, so callers pay the same cost as the previous local `++BUMP`. Kept untouched: bumpVersion / bumpOwnKeysSignal exports (consumed by mill/operators.ts) keep their existing signatures.
Self-review cleanups on top of the previous commit: - write.ts: hoist `Array.isArray(target)` to a single local `arr` (typed as `Array<unknown> | null`) instead of calling `isArray` 2-3× per setProperty/deleteProperty. Removes the awkward `(target as Array<unknown>)` casts in the length-signal blocks now that we have a properly-typed local. - for.ts: extract the "mirror src into dest in place" pattern (used at the initial seed and at two effect branches) into a single `copyArrayInto` helper. Three identical `prev.length = …; for (...) prev[i] = src[i]` blocks collapse to one call site each. No behavioral change.
…xyRef Four more allocation/work reductions in code I touched earlier: 1. for.ts swap effect: combine the per-index tracking pass with the change-detection pass into a single loop over `raw`. Tracking must run on every effect tick (to maintain alien-signals subscriptions), and change detection ran a second full pass when shapes matched. Folding them saves one O(N) pass per swap-effect tick. 2. for.ts non-parent path: combine the per-index tracking pass with the slot-build pass. Same idea — both iterate `raw.length` linearly, no reason to separate them. 3. for.ts elementCache: stop allocating a fresh `Map<unknown, ReactNode>` on every parent-path render. Each cache entry now carries a `gen` counter; renders bump the counter, restamp matched entries, and sweep anything with a stale `gen` at the end. Steady-state renders (swap, partial-update, select) allocate zero map entries per cache hit and never re-allocate the Map itself; only previously-unseen rawItems pay one wrapper allocation. 4. collections.ts: drop the `proxyRef` indirection. Map/Set methods that used to capture `proxyRef` and return it now type their `this` parameter and `return this`, matching native Map.prototype.set semantics. The handler's `$PROXY` symbol case reads through the intrusive `target[$PROXY]` cache (with sealedCollectionCache fallback) instead. Eliminates one mutable variable + assignment per reactive Map/Set creation.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## copilot/reduce-kernel-allocations #80 +/- ##
====================================================================
Coverage ? 99.24%
====================================================================
Files ? 31
Lines ? 1323
Branches ? 267
====================================================================
Hits ? 1313
Misses ? 4
Partials ? 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors performance-critical parts of @supergrain/kernel reactivity by centralizing monotonic “bump” writes, optimizing reactive array mutator wrapping, and tightening several hot loops (notably the React For component), with a couple of formatting cleanups in the benchmark package.
Changes:
- Centralize signal bumping via
nextBump()incore.tsand update writers/collections to use it. - Replace per-array mutator wrapper caching with a shared module-level wrapper table in
read.ts. - Optimize
For’s swap effect and element caching to reduce allocations and passes.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/kernel/src/core.ts | Adds shared nextBump() monotonic counter; removes array mutator symbol export. |
| packages/kernel/src/write.ts | Switches version/ownKeys bumping to nextBump() and consolidates write-side bump logic. |
| packages/kernel/src/read.ts | Introduces module-level array mutator wrapper table to avoid per-array caches. |
| packages/kernel/src/collections.ts | Updates Map/Set reactive wrappers to use nextBump() and refactors method return/proxy access patterns. |
| packages/kernel/src/react/for.ts | Adds in-place array copy helper and refactors swap detection + cache reuse for fewer allocations. |
| packages/js-krauset/perf-stats.ts | Formatting-only changes. |
| packages/js-krauset/package.json | Indentation/formatting normalization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return wrapper; | ||
| if (typeof prop === "string") { | ||
| const wrapper = arrayMutatorWrappers[prop]; | ||
| if (wrapper) return wrapper; |
There was a problem hiding this comment.
Array mutator wrapping now always returns a shared wrapper (built from Array.prototype[name]) whenever the property name matches, even if the array instance has an overridden method at that property. That changes semantics: proxy.push would ignore an own target.push implementation and call the built-in mutator instead. Consider only returning the shared wrapper when the resolved value is exactly the intrinsic Array.prototype[prop] (or otherwise fall back to returning value) so custom/overridden methods keep working.
Suggested change
| if (wrapper) return wrapper; | |
| if ( | |
| wrapper && | |
| value === | |
| (Array.prototype as unknown as Record<string, unknown>)[prop] | |
| ) { | |
| return wrapper; | |
| } |
| } | ||
|
|
||
| return proxyRef; | ||
| return this; |
There was a problem hiding this comment.
methods.set now returns this. Because these method functions are returned unbound from the proxy get trap, extracting the method (e.g. const set = mapProxy.set; set(k, v)) will run with this === undefined, so the mutation will succeed (it closes over rawTarget) but the return value will be undefined, breaking chaining and changing behavior from the previous proxyRef approach. To keep behavior stable, consider returning the proxy from the raw target’s $PROXY slot (or the sealed-cache fallback) instead of relying on this binding.
Suggested change
| return this; | |
| return ((rawTarget as ReactiveTagged<Map<K, V>>)[$PROXY] as Map<K, V> | undefined) ?? this; |
Comment on lines
369
to
372
| add: function reactiveAdd(this: Set<T>, value: T): Set<T> { | ||
| const rawValue = unwrap(value); | ||
| if (rawTarget.has(rawValue)) return proxyRef; | ||
| if (rawTarget.has(rawValue)) return this; | ||
|
|
There was a problem hiding this comment.
methods.add returns this (including the early return). Since the proxy get trap returns these functions unbound, extracting/calling add without a receiver will make this undefined and return undefined (even though the mutation happens via the rawTarget closure). This is a behavioral change from returning the proxy explicitly and can break chaining. Consider returning the stored proxy from rawTarget[$PROXY] (or sealed-cache fallback) rather than relying on this.
Benchmark numbers from a 20-run trimmed comparison showed the
version-tagged elementCache regressed create-many-rows (10k) by +6.7%
(significant; ~2 stddev) and replace-all-rows by +2.3% (significant).
The 10k regression accounted for ~all of the +4.8% weighted total
regression on its own.
Hypothesis: at large N, the per-item `{ node, gen }` wrapper allocation
plus the O(N) sweep at the end of the render outweighed the saving from
not reallocating the Map. Allocating one fresh Map of N entries lets V8
free the previous one in bulk via GC, which is apparently cheaper than
N restamps + N sweep iterations.
The other commits in this branch (For loop combining, write.ts node
lookup consolidation, BUMP consolidation, proxyRef → this in
collections.ts, global array mutator wrappers, hoisted Array.isArray,
copyArrayInto helper) are kept — none of them appear in the regression
profile. The 1k path (-8.7%) and clear-rows (-5.8%) show the net is
still positive elsewhere.
Lesson logged in spirit (not yet a notes/failed-approaches/ doc): a
version-tagged ephemeral cache is *not* a free win at scale. The wrapper
+ sweep cost dominates the per-render Map allocation it was trying to
avoid.
Two small changes in tracked() targeting the per-Row-render hot path: 1. Hoist the unmount closure into first-render setup, stored on `__sg.onUnmount`. Previously the arrow function passed to `useDisposeOnUnmount(...)` was reallocated on every render — for js-framework-benchmark partial-update that's 100 fresh closures per tick. The closure only references `forceUpdate`, which React guarantees is stable, so it's safe to freeze on first render. 2. Pre-bake an `effectSetup` function for the production path and call `useEffect(effectSetup, [])` directly instead of going through `useDisposeOnUnmount`. Saves one hook per render (the `useRef` indirection inside `useDisposeOnUnmount`) plus its `cleanupRef.current = cleanup` writeback. The dev path still uses `useDisposeOnUnmount` so React 18 StrictMode's mount→cleanup→remount cycle still defers the alien-effect teardown via the existing setTimeout(0) mechanism. The win per Row-render is small: roughly 1 hook call + 1 closure allocation. For partial-update (100 row re-renders) that's 100 fewer hooks and 100 fewer closures per tick. Whether that translates to measurable benchmark movement is unknown until run; flagging it as "easy win, worth measuring" rather than claiming a number.
Self-review on the previous commit:
- Drop the defensive `if (sg)` check in `onUnmount`. `__sg` is
guaranteed set by the time `onUnmount` runs (it's assigned in the
same first-render block, and dev StrictMode's deferred cleanup
preserves it through the mount→cleanup→remount cycle), so a
non-null assertion matches the original implementation's strictness.
- Use `fu` directly inside `onUnmount` instead of re-casting
`forceUpdate as unknown as { __sg?: TrackedState }` twice. `fu` is
the same object reference (just a typed alias) and is captured via
the first-render closure scope.
No behavior change.
Self-review readability pass: - `fu` → `dispatchHost`: it's the React useReducer dispatch function used as a host for our per-instance state, so name it for what it does. - `__sg` (string-keyed magic property) → `[TRACKED_STATE]` symbol: symbol-keyed slot is unambiguous, doesn't collide with anything React might attach to dispatch, and reads as "tracked state" at every use site instead of "supergrain abbreviation." - Capture the slot in a local `trackedState` once, after the first-render setup branch, instead of repeatedly indexing `dispatchHost[TRACKED_STATE]` in the hot path. No behavior change.
Two semantic regressions flagged by the copilot reviewer on PR #80: 1. read.ts: the global array mutator wrapper was returned for every matching property name, even when the array's `.push` (or other mutator) had been overridden by the caller. Now only swap in the wrapper when the resolved value is exactly `Array.prototype[prop]`, so user overrides and Array subclasses keep their own implementations. 2. collections.ts: methods like `Map.prototype.set` returning `this` (instead of the captured `proxyRef`) made extracted method calls silently break — `const set = mapProxy.set; set(k, v)` would mutate correctly but return `undefined` (not the proxy), breaking chaining. Reverted to the `proxyRef` pattern for both Map and Set. The intermediate "this" approach didn't move benchmarks anyway, so this is pure correctness with no perf cost.
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.
Summary
This PR consolidates and optimizes signal version bumping and array mutator wrapping across the codebase, eliminating redundant code and improving performance through shared module-level utilities.
Key Changes
Signal Bumping Consolidation
nextBump()function incore.tsinstead of maintaining separateBUMPcounters inwrite.tsandcollections.tsnextBump()functionArray Mutator Wrapping Optimization
$MUTATORScache (stored as hidden properties viaObject.defineProperty) with a module-levelarrayMutatorWrapperstable inread.tsgetMutatorCache()function and its associated property definition overheadthisbinding instead of closures, avoiding per-array allocation costs$MUTATORSsymbol export fromcore.tsas it's no longer neededReactive Collections Refactoring
proxyRefvariable pattern withthisbinding in Map and Set method implementations$PROXYlookups to read from the intrusive property on the target instead of maintaining a separate referencethis: Map<K, V>/this: Set<T>type annotationsReactiveTaggedtype usage where appropriateFor Component Optimization
copyArrayInto()helper to mutate arrays in-place instead of allocating new arrays with spread syntaxchangedarray with scalaraIdx/bIdxvariables andchangedCountcountergenstamp for reuse across renders instead of allocating a new Map each timeCode Quality
perf-stats.tsfor consistencypackage.jsonImplementation Details
nextBump()function is trivial enough that V8 will inline it, so callers pay the same cost as a local++BUMPhttps://claude.ai/code/session_01VDVFSh8xJqvYp61dZQ8q4o