Repository navigation
Conversation
The CodSpeed analysis runner times `await fn()`; a subject returning a merge/omit view had its `.then` probed through the proxy (and store) traps inside the timed window. Assign results to a module-level sink. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Size (brotli, eager entry chunk)
|
Coverage Report for CI Build 37835203882Coverage remained the same at 76.43%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Merging this PR will regress 10 benchmarks
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | readAllowed |
24.9 µs | 337.4 µs | -92.61% |
| ❌ | memo + sync render effect only (reference) |
28 ms | 32.3 ms | -13.28% |
| ❌ | readBlocked |
14.6 µs | 16.1 µs | -9.18% |
| ❌ | read |
24.4 µs | 26.7 µs | -8.63% |
| ❌ | hasAllowed |
20.6 µs | 22.5 µs | -8.41% |
| ❌ | read |
21.4 µs | 23 µs | -7.01% |
| ❌ | hasAllowed |
20.1 µs | 21.3 µs | -5.52% |
| ❌ | readBlocked |
13.5 µs | 14.3 µs | -5.19% |
| ❌ | readBlocked |
13.6 µs | 14.3 µs | -5.17% |
| ❌ | hasAllowed |
20.5 µs | 21.6 µs | -5.17% |
| ⚡ | merge |
819.8 µs | 27.4 µs | ×30 |
| ⚡ | construct |
341.1 µs | 30.3 µs | ×11 |
| ⚡ | merge |
42.5 µs | 21.8 µs | +94.45% |
| ⚡ | merge |
39.8 µs | 20.9 µs | +90.1% |
| ⚡ | merge |
39.7 µs | 20.9 µs | +89.66% |
| ⚡ | merge |
39.6 µs | 21 µs | +88.96% |
| ⚡ | merge |
39 µs | 20.7 µs | +88.19% |
| ⚡ | omit |
36.1 µs | 19.2 µs | +88.13% |
| ⚡ | merge |
38.9 µs | 20.7 µs | +87.48% |
| ⚡ | omit |
38 µs | 20.3 µs | +87.11% |
| ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing bench/construct-no-thenable-probe (7551870) with next (893834c)
A shared sink took objects, arrays, booleans and numbers across subjects; the primitive-returning read subjects measured 5-9% slower under it. Co-authored-by: Cursor <cursoragent@cursor.com>
This reverts commit 2e46cf6.
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.
Summary
packages/signals/tests/store/utilities.bench.tswraps every subject as() => subject.func(...args), so the benchmark function returns its result. CodSpeed's analysis runner timesawait fn()(@codspeed/vitest-plugin,runAnalysisBench). When the result is an object,awaitreads its.thenbeforestopBenchmark(). For theconstructsubjects the result is a merge or omit view, so that read goes through the view'sgettrap, and for store-backed inputs through the store's read path, all inside the timed window.This PR makes the subject wrapper a block body that assigns the result to a module-level
sink. The construction stays reachable, so it can't be eliminated as dead code, andawaitseesundefined. Nothing else about what each subject measures changes.Evidence (#3915)
#3915 showed
omit-proxy-store(25, 5) › constructgoing from 341.1 µs to 373.2 µs (−8.6%). CodSpeed's call graphs for that run againstnext(893834c) show:func → omitcosts about 20 µs on both sides.omit,OmitView,recordOf,readSource,isHidden,viewSource,getObserverandisWrappableall match.get(store/utils.js, the omit view's trap) sits outside the subject's call tree. It callsreadSource,getObserverandisWrappable, which is the.thenprobe going through the store. Its self cost is 308.8 µs on base and 340.8 µs on head, about 90% of the benchmark, and it holds the whole delta along with all of the run's system calls (8 to 10). That's one-off engine work landing in the window the probe holds open, not work in the subject.omit-proxy-store(100, 5) › constructruns the same code on four times the keys and costs 33.8 µs on both sides, with no straygetroot. So about 90% of the (25, 5) figure is the probe window, not construction.Benchmarks touched
All of them are in
packages/signals/tests/store/utilities.bench.ts. The wrapper is shared, so every subject in the file now goes through the sink:omit-static(…),omit-signal(…),omit-mixed(…)›omitmerge-static(…),merge-signal(…),merge-mixed(…)›mergemerge-merge-static(…),merge-merge-signal(…),merge-merge-mixed(…)›mergeomit-proxy-store(…)›construct,readAllowed,readBlocked,hasAllowed,ownKeysmerge-proxy-keys-store(…)›construct,ownKeys,readBaselines for these will shift once. The other bench files already use block bodies or a local
sink, and none of them returns a proxy toawait. They're unchanged.CodSpeed result on this PR
All changes are in
utilities.bench.ts. Outside it, nothing moved beyond noise (dbmon shallow full tickchanged 2–5% and was marked NoChange).omit/merge/merge-mergesubjects run about 37–44 µs onnextand about 18–27 µs here. The probe was roughly 15–20 µs of each measurement. Theconstructsubjects on store-backed inputs dropped 15–38%, andomit-proxy-store(25, 5) › constructwent from 341.1 µs to 30.4 µs.omit-proxy-store(25, 5) › readAllowedwent from 24.9 µs to 337.7 µs. Its call graph shows the same signature the straygetroot had onnext: about 205 µs of instructions plus 8 system calls, now as self cost inserveDataKey(store/store.js). Onnext, the.thenprobe was just the first store-trap read in that group's timed windows, so the one-off landed inconstruct. With the probe gone, the first read isreadAllowed's. The one-off is engine work tied to that first read, not the probe and not work in the subject. Moving it out of the timed windows entirely would need the group to warm the store read path before measuring, which this PR doesn't do.readBlockedandhasAllowed5–9%,merge-proxy-keys-store › read7–9%. Their call graphs show the extra cost as self time in the omit view'sgetand inisHidden. That's consistent with the trap's inline caches no longer having been primed by theconstructprobe; it isn't new work. Giving each subject its ownsinkmade no difference (tried and reverted on this branch).Public API changes
None. This is tooling only: no
packages/*/srcchanges, so there's no changeset.