fix(signals): held-input rules — a staged read enters the transaction, dependencies are the committed frame's (#3408, #3410) - #3431
Conversation
🦋 Changeset detectedLatest commit: dee461b 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 |
Coverage Report for CI Build 34889669512Coverage remained the same at 71.842%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Merging this PR will regress 1 benchmark
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | projection derive: delete + set one ROOT key (#3352) |
455.3 µs | 500.6 µs | -9.05% |
| ⚡ | projection derive: write one NESTED field (reference) |
523.8 µs | 224.7 µs | ×2.3 |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/held-conditional-memo (dee461b) with next (25c5064)
7b389f8 to
39c9a0d
Compare
|
@codspeedbot fix this regression
|
I reproduced the benchmark under the same instrument locally and bisected it. None of the three candidates is the cause, and there is nothing in the PR to fix — the 8.91% is a measurement artifact of how that bench file is built. What the flame-graph diff saysThe whole +44.5 µs is one frame's self time ( Local instruction counts (
|
| build | ROOT key (#3352) | NESTED (untouched) | createStore setter (untouched) |
|---|---|---|---|
next (da6ed76) |
537,655 | 180,890 | 513,411 |
| this PR (39c9a0d) | 623,679 | 181,599 | 513,424 |
| PR, re-run | 623,677 | 181,599 | 513,424 |
PR − enterStagedRead call sites |
623,361 | 181,182 | 512,508 |
PR − deps change (enterStagedRead kept) |
537,872 | 181,041 | 512,238 |
next + tail trimStaleDeps(el) only |
508,044 | 181,705 | 627,428 |
next + a provably dead statement |
537,454 | 180,663 | 511,585 |
Reproducible to ±2 instructions, so the +86,024 (+16% Ir) is real and not GC scheduling in the usual sense.
read()/enterStagedRead: +316 instructions (0.05%). Not it.- Reverting only the 2.0.0-rc.8 conditional memo disagrees with its inputs during a held branch change #3410 deps move restores the base number exactly (537,872 vs 537,655).
- But the cost is not the trim's work. In this benchmark the projection memo has exactly one dependency (measured: 1
link, 1trimStaleDeps, 0unlinkSubsper commit), the fan-in walk is 1 iteration and breaks on_depsTail, andtrimStaleDeps's own cost is 2,649 vs 2,879 instructions. Every other symbol in the window is bit-identical between base and head (wrapDraft45,499,flattenOverlay33,820,get11,254,setSignal6,849 …). No reactive-graph work changed.
What actually moves
A single ~86–120k-instruction chunk of one-off V8 work that attaches to whichever JS frame is running when it happens, and lands in exactly one of the three benchmark windows in the file.
The decisive control: add only the tail trimStaleDeps(el) call to the next tree, where it is a pure no-op because the original trim already ran three lines earlier. That makes the "regressed" benchmark 5.5% faster than base (508,044) and the untouched createStore setter benchmark 22% slower (627,428) — the same chunk, relocated. A dead statement in the same position moves nothing (537,454), so the trigger is the extra executed call perturbing V8's per-function tier-up budgets, not the source text.
That is also what happened in the report you're looking at: write one NESTED field, which this PR does not touch, moved ×2.4 (instructions 242.6 µs → 52.6 µs) in the same run. Locally the two builds differ by 0.4% on it.
Recommendation
Acknowledge the regression on CodSpeed — the changed code path costs ~300 instructions, not 86k. The instability is structural to projection-root-write.bench.ts: three benchmarks share one process and one set of 20k-key fixtures, and each body performs a single commit, so one iteration is measured while V8's tier-up state is still moving, and a ~100k chunk is ~20% of it. If you want the guard to be trustworthy at this resolution, having each body run N commits (or a warmup loop) would amortize it — happy to push that as a separate change if you want it, since it shifts the absolute numbers and re-baselines those three benchmarks.
No changes committed to this branch.
Summary
- Warming up the performance wizard...
- Locate base and head CodSpeed runs — head 39c9a0d (6aa82f6e) vs base da6ed76 (6aa823af).
- Compare flame graphs for the regressed benchmark — Whole +44.5 µs delta sits in one frame's self time; every callee subtree flat within 2 µs.
- Attribute the regression — Bisected: enterStagedRead costs +316 instr; the deferred trimStaleDeps triggers a floating ~86-120k V8 chunk that lands in an arbitrary benchmark window.
- Reproduce locally under Valgrind — Reproduced deterministically: 537,655 → 623,679 instructions (±2 across repeats).
- Report findings — No source change needed; the measured cost is not in the changed code path.
- Handling comment — Comment handling finished successfully
Learnings updated. See what CodSpeed remembers about this repository.
Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.
39c9a0d to
9307d53
Compare
…, dependencies are the committed frame's (#3408, #3410) Two sides of one gap. The engine already says "the staged world belongs to its transaction" for writes (setSignal on a stamped node enters it) and for stamped recomputes (recompute's head enters it). Reads and dependency trimming did not. a signal a transaction holds. Non-stale readers are served the staged value (readers keep speculation), but nothing joined the reader to the hold, so it published a value derived from the held world into the mainline frame (`Panel: 1` beside `Count: 0`). `enterStagedRead`: a tracked computation served a live transaction's `_pendingValue` enters the transaction, on each of read()'s value selections (both fast paths and the slow path). No-op for the ambient batch, the active transaction, and probe reads. stopped reading count and trimmed the edge, so a later count write never reached the memo whose committed value still derived from it (`Count: 1` beside `Selected: 0` / `Fixed: false`). Dependencies are the committed frame's until the frame is replaced — the deps twin of held children (#3404): a pass that staged its value leaves the previous pass's tail linked and `commitPendingNode` trims it (after a clean pass; `_error` set means the last pass threw and `_depsTail` marks where it stopped). The write then dirties the stamped memo, which enters the hold, exactly as an unconditional count() read does. Decided at commit because a plain flush knows nothing at recompute time: the transaction that holds the pass may open later in the same flush. Both now reveal atomically at the landing. +123 B core floor (conscious bump), eight brotli caps ratcheted by 0.05 KB with notes. No existing expectation moved. Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com>
9307d53 to
dee461b
Compare
…dy (#3435) Three benchmarks share one process and one set of 20k-key fixtures, and each body performed a single commit. V8 emits a one-off ~100k-instruction tier-up chunk that attaches to whichever frame is running when it fires and lands in exactly one of the three windows — at ~540k Ir per root write that is 16-20% of a window, and it moves between windows on unrelated changes. CodSpeed's bisection of #3431 (2026-09-14) made this exact: a change costing ~300 instructions on its own path read as -8.9% on the root write and x2.4 on the untouched nested write; a provably no-op extra call added to `next` read as -5.5% on the root write and +22% on the untouched store setter (537,655 → 508,044 and 513,411 → 627,428 Ir; every reactive-graph symbol bit-identical). Each body now runs COMMITS = 20 commits. The chunk amortizes to <1% of a window — below the 5% gate — and the three stay per-commit comparable, which is the parity this file guards (#3352 derive vs #3044 setter floor). Absolute numbers re-baseline ×20. Co-authored-by: Claude via Cursor <noreply@cursor.com> Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Fixes #3408, fixes #3410.
Two sides of one gap. The engine already says "the staged world belongs to its transaction" for writes (
setSignalon a stamped node enters it) and for stamped recomputes (recompute's head enters it). Reads and dependency trimming did not.#3408 — a memo that starts reading a held signal revealed its staged value
setCount(1)is held bydetails.setShow(true)flipspanelmainline; it now readscount, and non-stale readers are served the staged value (readers keep speculation — by design). But nothing joinedpanelto the hold, so it published a value derived from the held world into the mainline frame:Panel: 1besideCount: 0.Rule: a tracked computation served a live transaction's
_pendingValueenters that transaction (enterStagedRead), on each ofread()'s value selections (both fast paths and the slow path). No-op for the ambient batch, the active transaction, and probe reads (pendingCheckActive). It's the read twin of the existing write-side and recompute-side entries.#3410 — a memo whose held pass trimmed an input disagreed with its committed inputs
setFixed(true)is held.selected's held pass stops readingcountandtrimStaleDepsdropped the edge — sosetCount(1)never reached a memo whose committed value still derived fromcount:Count: 1besideSelected: 0/Fixed: false.Rule: dependencies are the committed frame's until the frame is replaced — the deps twin of held children (#3404). A pass that staged its value leaves the previous pass's dependency tail linked;
commitPendingNodetrims it (after a clean pass —_errorset means the last pass threw and_depsTailmarks where it stopped). Thecountwrite then dirties the stamped memo, which enters the hold, exactly as an unconditionalcount()read does. Decided at commit rather than recompute because a plain flush knows nothing at recompute time: the transaction that holds the pass may open later in the same flush.__OBSERVE__fan-in counting now walks only the validated prefix so held tails don't inflate distinct-source counts.Both scenarios now reveal atomically at the landing (
tests/held-conditional-memo.test.ts).Verification
@solidjs/signals: 167 files / 1785 tests green; full monorepoturbo test --concurrency 1green for every package except 3@solidjs/compilerparity tests that fail identically onorigin/next(compiler/babel-plugin trees are untouched here).treeshake.test.ts); eight brotli caps ratcheted by 0.05 KB with notes inscripts/size/.size-limit.js.src/typechecks clean. No existing expectation moved.SPEC-ASYNC-SEMANTICS.md, each with Status / Pinned by / Mechanism; cited by ID fromenterStagedRead,recompute's tail,commitPendingNodeand the two pins, so the rules-index gate covers them (RULES-INDEX.mdregenerated: 307 rules, every src citation resolves, every live A-rule test-cited). A28 is fix(signals): writes become visible at flush — latest() reads the flushed staged world (A28) #3337's. TheINTERNALS-ASYNC-STATE.md§3 bullets are reduced to mechanism notes pointing at the rules. Changeset included.Rebased on
nextafter #3432 / #3433.Co-authored-by: Claude noreply@anthropic.com