perf(web,signals): spread() reads a merge() proxy through its sources - #3325
Merged
Merged
Conversation
🦋 Changeset detectedLatest commit: e01022e The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Merging this PR will improve performance by 25.37%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | spread merge(static, reactive) × 500 |
24.4 ms | 16.6 ms | +47.18% |
| ⚡ | spread store record × 500 |
35.2 ms | 33 ms | +6.8% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing perf/spread-direct-sources (e01022e) with next (3a5fe8c)
ryansolid
marked this pull request as draft
September 9, 2026 20:04
Member
Author
|
A spread mixed with other attributes compiles to `spread(el, merge(statics, () => rest))`. Going through the merge proxy costs merge's `keys()` (a Set plus an own-enumerable scan of every source) and then, per key, a right-to-left `in` walk of the sources, every run. The spread now iterates the flattened sources directly — own string keys, later sources overriding earlier, children/ref excluded — and enumerates each source through the same single-trap `ownKeys` path readShallow() uses (#3326). omit() is not a merge and stays opaque. Rebased over #3326, which landed the shared proxy enumeration and the tracked style/class read; the style() proxy branch and the per-source descriptor-trap fix from the original commit are subsumed by it and dropped here, as is the duplicate spread-style bench (spread-enumerate's merge row is the guard). @solidjs/signals gains an @internal mergeSources(). Co-authored-by: Cursor <cursoragent@cursor.com>
ryansolid
force-pushed
the
perf/spread-direct-sources
branch
from
September 10, 2026 01:00
f1e297c to
7ad9354
Compare
Copy-on-write for the array branch bought identity passthrough for
proxy-free class arrays at the cost of a slice/push loop. className()
already allocates two objects per array value, so the one extra array from
value.map(readShallow) is invisible: paired Tier-1 bench, 500 elements with
class={[...]}, 1,981 vs 1,981 ops/s. −243 B raw / −58 B brotli on web.js.
Co-authored-by: Cursor <cursoragent@cursor.com>
merge() has two output forms. The proxy form's writes are no-ops, so its sources are the whole truth. The plain-object form (all sources plain) also records $SOURCES so nested merges flatten — but it is a real object callers mutate afterwards: @solidjs/html builds `props = merge(props, spread)` and then assigns later props onto the result. Reading that form through its sources missed those own writes (html: "handles multiple spread attributes with complex override behavior" — class stayed "override" instead of "final"). mergeSources() now returns undefined unless the object is the proxy itself; spread() reads the plain form directly, as before. Pinned in spread-sources.spec with html's shape. Co-authored-by: Cursor <cursoragent@cursor.com>
Coverage Report for CI Build 34437797850Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage remained the same at 71.851%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
ryansolid
marked this pull request as ready for review
September 10, 2026 05:11
ryansolid
added a commit
that referenced
this pull request
Sep 15, 2026
…internal `merge()`/`omit()` returning lazy views (#3454) gave `spread()` and `ssrElement()` a protocol for reading props leaf by leaf instead of trapping through the proxy per key. `@solidjs/web` and `@solidjs/universal` depend on `solid-js` alone — never on `@solidjs/signals` directly, so an app holds exactly one reactive engine — so every piece of that protocol went out through `solid-js`'s main export: eleven names, `mergeSources` before them in #3325, on the public surface with no marking. None of it is API. `solid-js/internal` is now their home: the view protocol (`viewOf`, `mergeView`/`omitView`, `MergeView`/`OmitView`, `sourceKeys`/`sourceHas`/ `sourceGet`, `hasStaticKeys`, `resolvedTable`, the `SOURCE_*` kinds), and the server-scope seams that were already `@internal` in JSDoc and consumed only by `@solidjs/web` (`ssrHandleError`, `ssrScope`, `runInServerComponentScope`, `inServerComponentScope`, `creationStamp`, `getProjectionTrace`, `materializeContainerTrace`). The names stay exported from the main entries AT RUNTIME, so the subpath shares one module state and the single-engine guarantee is untouched; they carry `@internal` and `stripInternal` keeps them out of the generated declarations, which is what makes them not-public for TypeScript. The protocol half re-exports `@solidjs/signals` as-is; the seams are read back from "solid-js" (external, so the platform/tier conditions pick the same build the app runs) and declared with their signatures spelled out, since the main entries no longer type them. Dropped from the entries as dead: `storeIsShallow`, `storeHasFamily`, `storeHasOptimisticFamily` (leftovers of the gutted patch channel), `storePath` and `$REFRESH` (referenced only by `@solidjs/signals`'s own internals), `NoHydrateContext` (`@internal`, used only by `solid-js`'s server code). Nothing in the repo consumed them. internal-surface.spec.ts pins the boundary in both directions — no internal name in either main entry's declarations, all of them in internal.d.ts — which is the check that would have caught #3454 leaking eleven of these. No runtime behavior change. Size, measured on real app bundles: the client is byte-identical raw and 13 bytes smaller gzipped (the seams tree-shake out entirely, so the namespace import costs nothing), the server is +24 gzipped from two alias consts the typed declarations require. solid-js's own entries shrink 122/141 gzipped bytes from the dead exports. Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ryansolid
added a commit
that referenced
this pull request
Sep 15, 2026
…internal (#3470) * chore(solid,web,universal): the runtimes' seams move behind solid-js/internal `merge()`/`omit()` returning lazy views (#3454) gave `spread()` and `ssrElement()` a protocol for reading props leaf by leaf instead of trapping through the proxy per key. `@solidjs/web` and `@solidjs/universal` depend on `solid-js` alone — never on `@solidjs/signals` directly, so an app holds exactly one reactive engine — so every piece of that protocol went out through `solid-js`'s main export: eleven names, `mergeSources` before them in #3325, on the public surface with no marking. None of it is API. `solid-js/internal` is now their home: the view protocol (`viewOf`, `mergeView`/`omitView`, `MergeView`/`OmitView`, `sourceKeys`/`sourceHas`/ `sourceGet`, `hasStaticKeys`, `resolvedTable`, the `SOURCE_*` kinds), and the server-scope seams that were already `@internal` in JSDoc and consumed only by `@solidjs/web` (`ssrHandleError`, `ssrScope`, `runInServerComponentScope`, `inServerComponentScope`, `creationStamp`, `getProjectionTrace`, `materializeContainerTrace`). The names stay exported from the main entries AT RUNTIME, so the subpath shares one module state and the single-engine guarantee is untouched; they carry `@internal` and `stripInternal` keeps them out of the generated declarations, which is what makes them not-public for TypeScript. The protocol half re-exports `@solidjs/signals` as-is; the seams are read back from "solid-js" (external, so the platform/tier conditions pick the same build the app runs) and declared with their signatures spelled out, since the main entries no longer type them. Dropped from the entries as dead: `storeIsShallow`, `storeHasFamily`, `storeHasOptimisticFamily` (leftovers of the gutted patch channel), `storePath` and `$REFRESH` (referenced only by `@solidjs/signals`'s own internals), `NoHydrateContext` (`@internal`, used only by `solid-js`'s server code). Nothing in the repo consumed them. internal-surface.spec.ts pins the boundary in both directions — no internal name in either main entry's declarations, all of them in internal.d.ts — which is the check that would have caught #3454 leaking eleven of these. No runtime behavior change. Size, measured on real app bundles: the client is byte-identical raw and 13 bytes smaller gzipped (the seams tree-shake out entirely, so the namespace import costs nothing), the server is +24 gzipped from two alias consts the typed declarations require. solid-js's own entries shrink 122/141 gzipped bytes from the dead exports. Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com> * fix(size): route solid-js/internal in the size scenarios esbuild's `alias` matches by PREFIX, so the bare `solid-js` entry remapped `solid-js/internal` to `.../dist/solid.js/internal` and the prod and observe scenarios failed to resolve — the same trap this file already documents for `solid-js/attribution`. Subpath alias listed first in both maps, and added to the frames scenario's `external` (its client entry imports the seam). Two caps move, both measured against `next` on the same machine: - frames 11.40 -> 11.45 KB: 11442 B against 11370 (+72 brotli on +31 minified). Nothing in that bundle changed but one import's specifier — `materializeContainerTrace` used to fold into the single `from "solid-js"` statement and is now its own `from "solid-js/internal"` statement. - hydrating + store family 28.90 -> 28.95 KB: 28901 B against 28861 (+40). Mangler noise: the minified bundle is byte-identical (89072 B both sides) and differs only in which short names the minifier hands out, the extra module boundary having shifted its allocation. The same swap compresses simple-app -33, hydrating -45 and CSR -21; this one came out 1 B over a cap #3459 had just used the room under. The other seven scenarios are unchanged or smaller. Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com>
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.
Rebased over #3326. What remains here is the one piece #3326 did not cover: reading a
merge()proxy through its flattened sources rather than through the proxy.A spread mixed with other attributes compiles to
spread(el, merge(statics, () => rest)). Onnext, that source now costs oneownKeystrap — but for a merge proxy the trap is merge'skeys(): aSetplus an own-enumerable scan (Reflect.ownKeys+propertyIsEnumerablefilter) of every source; then every key'sgetis a right-to-leftinwalk of the sources. Iterating the sources directly removes all of it: own string keys per source, later sources overriding earlier (merge's own contract),children/refexcluded, each source enumerated through the same single-trapownKeyshelperreadShallow()uses.omit()is not a merge and stays opaque (enumerated through its own filtering trap).Dropped from the original commit as subsumed by #3326: the
style()proxy branch (style()now receives a plain copy fromreadShallow), thecollectPropsper-source descriptor-trap fix (now the sharedownKeys()), and thespread-stylebench (themerge(static, reactive)row ofspread-enumerate.bench.tsxis the guard).Semantics pinned in
spread-sources.spec.tsx: later-wins,children/ref/symbols excluded,omit()inside a merge and alone, store key add/remove tracked, whole-object replace, own-properties-only. One documented contract: own keys per source — a key an earlier source owns and a later source merely inherits resolves to the earlier source (the proxy'sinwalk saw the inherited one); spread has always applied own properties only.Size: ~+50 B on the app scenarios vs
next; all limits hold.Perf (Tier-1
spread-enumerate, 500 elements × 8 keys, median of 3 runs each, paired on the same machine):next(after #3326)merge(static, reactive)The merge row is the change; it now sits within ~25% of the plain-object floor. The two untouched rows moved together by ~10%, which is machine drift between the paired runs, not the patch — the merge row's 2× is far outside it.