Skip to content

perf(web): resolve condition memos inline in a spread's collect - #4566

Closed
brenelz wants to merge 1 commit into
solidjs:nextfrom
brenelz:perf/spread-view-layer-effects
Closed

brenelz wants to merge 1 commit into
solidjs:nextfrom
brenelz:perf/spread-view-layer-effects

Conversation

@brenelz

@brenelz brenelz commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Reduced to the memo() change after review. The position-diff path is gone, and each element still has one attribute effect.

What was slow

On the Kobalte-shaped polymorphic chain (test/harness/polymorphic.tsx), a label change reruns the spread's attribute effect, which reruns every getter. Two of those getters contain compiled conditions, memo(() => !!isNativeButton())(), so each run created two memo nodes and disposed the previous run's two.

What changed

memo() in render.ts returns the condition thunk when it is created directly under the spread effect that is collecting props (setCollector in spread()). The effect already reruns every getter, so the memo only cost a node per run. During hydration the memos are kept, because they take hydration ids.

There is a trade. A condition's inputs become dependencies of the attribute effect, so an input change that does not flip the condition now reruns every getter on that element. On next, only the per-run memo reruns. The repro from review shows 50 reruns where next shows 0.

Measurements

These are from a standalone harness on production builds: vite compiles the polymorphic harness, and Node with jsdom runs it with --conditions=browser. Each figure is the minimum over 40 to 60 batches, across three interleaved rounds on a loaded machine.

1000 rows next This PR
Update 100 rows x 16, chain 6.62–6.94 ms 5.27–5.54 ms
Mount and clear, chain 33.7–36.0 ms 32.3–34.6 ms

Size

Against next with node size.mjs --no-gate --json and node gate.mjs:

  • Compiled CSR grows by 101 B minified (40 B brotli), and compiled hydrating by 100 B (7 B brotli). Both scenarios are now over their brotli caps, compiled CSR by 31 B and compiled hydrating by 38 B, which was already over on next. The gate passes because both stay within the minified allowance.
  • The two router pages grow by 34 and 37 B minified.

Verification

  • In packages/web, npx vitest run, npx vitest run --config vite.config.server.mjs and npx vitest run --config vite.config.hydrate.mjs pass (1664, 1934 and 754 tests), as does pnpm run test-types. The packages/solid tests pass (921).
  • New specs in test/spread-nodes.spec.tsx: a rerun leaves no condition memo under the attribute effect (fails on next), and condition getters still track both the condition and the chosen branch.

🤖 Generated with Claude Code

@changeset-bot

changeset-bot Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 8bfa8ea

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 15 packages
Name Type
@solidjs/web Patch
@solidjs/babel-plugin Patch
@solidjs/diagnostics Patch
@solidjs/element Patch
@solidjs/h Patch
@solidjs/html Patch
test-integration Patch
board-example Patch
canvas-example Patch
server-dbmon-example Patch
todos-server-example Patch
@solidjs/compiler Patch
@solidjs/signals Patch
solid-js Patch
@solidjs/universal Patch

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

@codspeed

codspeed Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

Merging this PR will improve performance by 31.29%

2 warnings

⚠️ 3 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

⚠️ 2 benchmarks with a significant change ran on different hardware

Their base and head results were measured on machines with a different CPU, core count or memory, so the changes might not come from the code. Run base and head on the same runner type.

Learn more about Macro Runners

See affected benchmarks

⚡ 2 improved benchmarks
✅ 198 untouched benchmarks
⏩ 5 skipped benchmarks1

Performance Changes

Benchmark BASE HEAD Efficiency
⚡ polymorphic-chain update 100/1000 rows × 16: chain 110.1 ms 82.9 ms +32.69%
⚡ polymorphic-chain update 100/1000 rows × 16: chain-static 106.3 ms 81.9 ms +29.91%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.


Comparing brenelz:perf/spread-view-layer-effects (07fa8c2) with next (f8080f3)

Open in CodSpeed

Footnotes

  1. 5 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@brenelz

brenelz commented Nov 1, 2026

Copy link
Copy Markdown
Contributor Author

meh not sure its worth it. Targets one very specific benchmark

Copy link
Copy Markdown
Member

Thanks for this, @brenelz. Halving the update time on the polymorphic chain is a real win. Ryan read the review and wants a rework rather than landing it as is. Both of your open questions are the reasons:

  • The spread() creates three reactive nodes per element #3388 cap is a hard contract. Effect count per element is the cost we guard most closely. Going from 1 or 2 nodes to 3 or 4 per spread element shows up as a mount regression; you measured it yourself (33.3 → 35.6 ms mount/clear). In js-framework-benchmark terms, even 2 vs 1 effects per row has been a serious hit. So splitting the spread into one effect per props layer won't land.
  • Atomicity. An element's attributes need to keep updating as one unit. With per-layer effects, a pending getter in one layer lets the other layers apply in the same frame, which breaks the whole-frame rule (F-1).
  • Size. It adds about +1 KB on the compiled CSR and hydrating rows, and we're holding hello world under 10 KB.
  • Smaller issues. It changes the order attributes are applied in, and the job CI step fails on a test-types error in the new spec.

Where the time actually goes, per your own write-up: the two compiled memo(() => !!isNativeButton()) conditions are rebuilt on every run of the single effect, and all 13 getters rerun through the merge/omit layers. A rework that keeps one effect per element would be very welcome. For example:

  • stop those conditions being rebuilt per run, whether in the compiler output or in how the view resolves them;
  • cut the per-run cost of walking merge/omit layers in spread()/assign().

That way the update win stays without paying in mount time, effect count or bytes.


Generated by Claude Code

@brenelz
brenelz force-pushed the perf/spread-view-layer-effects branch from 5424730 to 8bfa8ea Compare November 1, 2026 17:16
@brenelz brenelz changed the title perf(web): rerun only the changed props layer in a view spread perf(web): skip condition memos and diff view values by position in spread Nov 1, 2026
@brenelz

brenelz commented Nov 1, 2026

Copy link
Copy Markdown
Contributor Author

Reworked along the lines you suggested, and force-pushed as one commit on current next.

  • Effect count. Each spread element keeps one attribute effect, so the spread() creates three reactive nodes per element #3388 cap holds and an element's attributes still apply as one unit. Application order is the same as before.
  • Condition memos. A compiled memo(() => !!cond)() created directly under the spread effect that is collecting props now evaluates inline instead of creating a node. During hydration the memos are kept, because they take hydration ids. I did this in the runtime rather than the compiler, because hoisting the memo out of the getter would have to change both compilers and keep server and client ids in step.
  • Merge/omit walk. For a view over plain objects with a fixed key set, the effect reads values into an array by position and calls assignProp only for the ones that changed. There is no props object per run and no removal pass. Views with a ref, and form elements, keep the general path.

Numbers on the chain, from production builds in a standalone jsdom harness:

next This PR
Update 6.17 ms 4.25 ms
Mount and clear 33.5 ms 32–33 ms

The compiled element is 3.0 ms on update. Most of the remaining gap is the 13 getters that still rerun on every change, which a single effect can't avoid.

Size is +182 and +153 B brotli on the compiled CSR and hydrating scenarios. The memo change on its own is +46 and +66 B brotli for about half the update win (6.17 to 5.23 ms), so I can drop the array path if the bytes aren't worth it. The test-types failure is gone.

@brenelz
brenelz marked this pull request as ready for review November 1, 2026 17:33

Copy link
Copy Markdown
Member

Thanks for the rework, @brenelz. It's a big step forward:

  • still one attribute effect per element;
  • attributes update as one unit;
  • application order is unchanged;
  • test-types passes;
  • on a prod build, chain updates are about 27–33% faster;
  • mount/clear is flat.

Two things still keep it from landing.

1. Condition memos no longer absorb input changes that don't flip them. memo() now returns fn directly while the collecting spread effect is the owner. So every input a condition reads becomes a direct dependency of the whole spread effect. On next, count() going 1 → 50 under count() > 100 reruns only the per-run memo. Here, every one of those writes reruns every getter on the element. The benchmark doesn't catch this because label updates always flip. Repro (0 reruns on next, 50 on this head):

/**
 * @jsxImportSource @solidjs/web
 * @vitest-environment jsdom
 */
import { expect, test } from "vitest";
import { memo, render, spread } from "@solidjs/web";
import { createSignal, flush, merge, omit } from "solid-js";

test("a condition memo still absorbs input changes that do not flip it", () => {
  const [count, setCount] = createSignal(0);
  let titleReads = 0;
  const props = merge(
    { id: "x" },
    omit({
      as: "div",
      get "data-big"() { return memo(() => !!(count() > 100))() ? "" : undefined; },
      get title() { titleReads++; return "t"; }
    }, "as")
  );
  const container = document.createElement("div");
  const dispose = render(() => { const el = document.createElement("div"); spread(el, props); return el; }, container);
  flush();
  const r0 = titleReads;
  for (let i = 1; i <= 50; i++) { setCount(i); flush(); }
  expect(titleReads - r0).toBe(0);
  dispose();
});

One direction: keep each condition memo alive across runs, for example one persistent memo per call position, instead of skipping it. The extra owner check also costs every memo() call, which is why the router rows grew.

2. Size. Compiled CSR grows +442 B minified (+182 B brotli) and compiled hydrating +438 B (+144 B brotli). That's mostly the separate position-diff path next to collectSpread. Bytes matter a lot here: apps that don't hit a path shouldn't pay for it. So it needs to fold into the existing path, aiming for roughly flat.

(The CodSpeed red is mount-rows "memo + sync render effect only (reference)", a signals-only bench this PR doesn't touch, so that's noise.)


Generated by Claude Code

@brenelz

brenelz commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks. I looked into both points.

  1. Condition memos. Confirmed: your repro gives 50 reruns. A persistent memo per call position isn’t safe at runtime. A getter like open() ? a.label : b.label puts a different condition in the same position when open flips. The only correct version re-tracks the memo on every run, which needs a hook in signals core and saves less.

  2. Size. The position diff alone is 6.17 to 5.35 ms. I couldn’t fold it in: skipping the removal pass saves only about 0.1 ms, and the rest of the win comes from the separate path not building a props object on each run.

@ryansolid

Copy link
Copy Markdown
Member

Thanks for this. We ran it through our own benchmarks: the Octane board and yak-bench, Chromium with 4× CPU throttling, interleaved against next. We'd like one more round before it's worth the bytes.

What we saw

  • Octane board, 9 client suites: −1.5% geomean, with no regressions that held up across rounds. But few board fixtures have the merge/omit spread shape.
  • yak-bench, 15 cases (including polymorphic-chain): hydrate and mount unchanged. The re-render case, which rebuilds every instance, was ~5% slower (1.06× against a 1.01× noise floor), and polymorphic-chain 8% slower (19.8 vs 18.4 ms).
  • That fits the position path's setup on every spread creation: copying and filtering the table into entries, the prev array, and the collecting closure. yak doesn't time updates to a mounted chain, so your jsdom update gains aren't contradicted, but creation pays for them.
  • Size: ~+440 B minified on compiled CSR and hydrating is a lot for a gain that only shows on this shape.

Suggestions

  1. Split out the memo() change: 136 B for about half the update gain, and no per-spread setup at creation. We'd likely take that on its own.
  2. If the position path stays, build entries and prev lazily on the first rerun, so mount and one-shot renders don't pay for it. Then check creation doesn't regress, for example with yak-bench's re-render case.

🤖 Generated with Claude Code

A spread's attribute effect reruns every getter it reads, so a compiled
`memo(() => !!cond)()` inside one was rebuilt on each run. Outside
hydration it now resolves inline, and the condition's inputs become
dependencies of the attribute effect.
@brenelz
brenelz force-pushed the perf/spread-view-layer-effects branch from 8bfa8ea to 07fa8c2 Compare November 2, 2026 22:12
@brenelz brenelz changed the title perf(web): skip condition memos and diff view values by position in spread perf(web): resolve condition memos inline in a spread's collect Nov 2, 2026
@brenelz

brenelz commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks. Building entries lazily took the setup off creation, but the folded branches made it bigger, +478 B minified against +442 B, so I've dropped the position path. This is now just the memo() change, on current next. Compiled CSR and hydrating grow by 100 B minified (+40 and +7 B brotli), and compiled CSR is now over its brotli cap by 31 B. Chain update goes from about 6.8 to 5.4 ms on a prod build, and mount is flat.

One thing to decide knowingly: the trade from your earlier repro is still there. A condition's inputs are now dependencies of the attribute effect, so an input change that doesn't flip the condition reruns every getter on the element (50 reruns in the repro, 0 on next). Let me know if that's acceptable for this gain.

Copy link
Copy Markdown
Member

Thanks for pushing on this. We're closing it rather than landing a version of it.

The blocker was that skipping the condition memos let one key's rerun absorb another key's condition. To fix that without skipping them, the memos would have to survive across the attribute effect's reruns. They can't be kept per call position: compiled getters are new closures on every run, and which conditions get called depends on earlier condition values (a() ? x : memo(b)() ? …), so positions shift and a reused memo could compute the wrong condition.

We built the safe alternative, one persistent memo per key. It fixed the absorb case and kept one effect per element, but it cost about +500 B minified on compiled CSR and hydrating, needed a hook in memo(), and changed how an element holds while a condition's async is in flight (D-2 to D-3). That's too much for a perf change, so spread stays as it is on next.


Generated by Claude Code

@ryansolid ryansolid closed this Nov 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants