Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 8bfa8ea The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 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 31.29%2 warnings
Performance Changes
Tip Curious why performance improved? Comment Comparing Footnotes
|
|
meh not sure its worth it. Targets one very specific benchmark |
|
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:
Where the time actually goes, per your own write-up: the two compiled
That way the update win stays without paying in mount time, effect count or bytes. Generated by Claude Code |
5424730 to
8bfa8ea
Compare
|
Reworked along the lines you suggested, and force-pushed as one commit on current
Numbers on the chain, from production builds in a standalone jsdom harness:
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 |
|
Thanks for the rework, @brenelz. It's a big step forward:
Two things still keep it from landing. 1. Condition memos no longer absorb input changes that don't flip them. /**
* @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 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 (The CodSpeed red is Generated by Claude Code |
|
Thanks. I looked into both points.
|
|
Thanks for this. We ran it through our own benchmarks: the Octane board and yak-bench, Chromium with 4× CPU throttling, interleaved against What we saw
Suggestions
🤖 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.
8bfa8ea to
07fa8c2
Compare
|
Thanks. Building 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 |
|
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 ( 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 Generated by Claude Code |
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()inrender.tsreturns the condition thunk when it is created directly under the spread effect that is collecting props (setCollectorinspread()). 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 wherenextshows 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.nextSize
Against
nextwithnode size.mjs --no-gate --jsonandnode gate.mjs:next. The gate passes because both stay within the minified allowance.Verification
packages/web,npx vitest run,npx vitest run --config vite.config.server.mjsandnpx vitest run --config vite.config.hydrate.mjspass (1664, 1934 and 754 tests), as doespnpm run test-types. Thepackages/solidtests pass (921).test/spread-nodes.spec.tsx: a rerun leaves no condition memo under the attribute effect (fails onnext), and condition getters still track both the condition and the chosen branch.🤖 Generated with Claude Code