Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Size Change: 0 B Total Size: 103 kB ℹ️ View Unchanged
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Benchmark React
Details
| Benchmark suite | Current: 5cd894c | Previous: b239883 | Ratio |
|---|---|---|---|
data-client: getlist-100 |
224.75 ops/s (± 5.0%) |
175.44 ops/s (± 3.7%) |
0.78 |
data-client: getlist-500 |
68.03 ops/s (± 5.0%) |
55.71 ops/s (± 4.0%) |
0.82 |
data-client: update-entity |
526.32 ops/s (± 10.6%) |
454.55 ops/s (± 8.4%) |
0.86 |
data-client: update-user |
625 ops/s (± 10.2%) |
454.55 ops/s (± 9.3%) |
0.73 |
data-client: getlist-500-sorted |
60.8 ops/s (± 8.6%) |
56.03 ops/s (± 9.8%) |
0.92 |
data-client: update-entity-sorted |
666.67 ops/s (± 7.0%) |
408.33 ops/s (± 7.1%) |
0.61 |
data-client: update-entity-multi-view |
645.83 ops/s (± 8.2%) |
416.67 ops/s (± 8.6%) |
0.65 |
data-client: list-detail-switch-10 |
21.41 ops/s (± 11.3%) |
14.71 ops/s (± 7.7%) |
0.69 |
data-client: update-user-10000 |
118.35 ops/s (± 16.5%) |
85.84 ops/s (± 16.4%) |
0.73 |
data-client: invalidate-and-resolve |
56.02 ops/s (± 4.8%) |
47.96 ops/s (± 5.5%) |
0.86 |
data-client: unshift-item |
294.12 ops/s (± 5.5%) |
263.16 ops/s (± 6.1%) |
0.89 |
data-client: delete-item |
476.19 ops/s (± 5.0%) |
357.14 ops/s (± 4.7%) |
0.75 |
data-client: move-item |
274.02 ops/s (± 10.0%) |
212.77 ops/s (± 8.3%) |
0.78 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Benchmark
Details
| Benchmark suite | Current: 5cd894c | Previous: b239883 | Ratio |
|---|---|---|---|
normalizeLong |
429 ops/sec (±3.45%) |
427 ops/sec (±3.13%) |
1.00 |
normalizeLong Values |
401 ops/sec (±0.35%) |
387 ops/sec (±0.86%) |
0.97 |
normalizeLong Scalar |
370 ops/sec (±2.94%) |
359 ops/sec (±3.70%) |
0.97 |
normalizeLong Scalar update |
900 ops/sec (±1.71%) |
921 ops/sec (±0.19%) |
1.02 |
denormalizeLong |
243 ops/sec (±5.78%) |
243 ops/sec (±6.27%) |
1 |
denormalizeLong Values |
231 ops/sec (±4.79%) |
224 ops/sec (±5.37%) |
0.97 |
denormalizeLong donotcache |
1004 ops/sec (±0.37%) |
1000 ops/sec (±0.80%) |
1.00 |
denormalizeLong Values donotcache |
739 ops/sec (±0.23%) |
749 ops/sec (±0.20%) |
1.01 |
denormalizeLong Scalar donotcache |
1049 ops/sec (±0.17%) |
1063 ops/sec (±0.17%) |
1.01 |
denormalizeShort donotcache 500x |
1404 ops/sec (±0.38%) |
1402 ops/sec (±0.12%) |
1.00 |
denormalizeShort 500x |
647 ops/sec (±6.22%) |
637 ops/sec (±6.57%) |
0.98 |
denormalizeShort 500x withCache |
7043 ops/sec (±0.43%) |
7080 ops/sec (±0.20%) |
1.01 |
queryShort 500x withCache |
3214 ops/sec (±0.65%) |
3193 ops/sec (±0.12%) |
0.99 |
buildQueryKey All |
58910 ops/sec (±0.89%) |
58045 ops/sec (±0.68%) |
0.99 |
query All withCache |
6038 ops/sec (±2.84%) |
6282 ops/sec (±2.69%) |
1.04 |
denormalizeLong with mixin Entity |
216 ops/sec (±6.29%) |
221 ops/sec (±7.08%) |
1.02 |
denormalizeLong withCache |
7233 ops/sec (±0.22%) |
7616 ops/sec (±0.17%) |
1.05 |
denormalizeLong withCache (Scalar churn) |
7195 ops/sec (±0.20%) |
7618 ops/sec (±0.20%) |
1.06 |
denormalizeLong Values withCache |
4900 ops/sec (±2.27%) |
5205 ops/sec (±1.58%) |
1.06 |
denormalizeLong Scalar withCache |
6230 ops/sec (±0.21%) |
7839 ops/sec (±0.39%) |
1.26 |
denormalizeLong Scalar update withCache |
3549 ops/sec (±0.19%) |
4075 ops/sec (±0.28%) |
1.15 |
denormalizeLong All withCache |
6134 ops/sec (±0.73%) |
6710 ops/sec (±0.61%) |
1.09 |
denormalizeLong Query-sorted withCache |
6303 ops/sec (±1.79%) |
6598 ops/sec (±1.48%) |
1.05 |
denormalizeLongAndShort withEntityCacheOnly |
1726 ops/sec (±0.22%) |
1813 ops/sec (±0.26%) |
1.05 |
denormalize bidirectional 50 |
4642 ops/sec (±11.05%) |
4537 ops/sec (±10.97%) |
0.98 |
denormalize bidirectional 50 donotcache |
41308 ops/sec (±1.77%) |
42714 ops/sec (±0.48%) |
1.03 |
getResponse |
4545 ops/sec (±4.03%) |
4440 ops/sec (±3.68%) |
0.98 |
getResponse (null) |
10001277 ops/sec (±0.62%) |
9674787 ops/sec (±0.75%) |
0.97 |
getResponse (clear cache) |
211 ops/sec (±6.13%) |
208 ops/sec (±9.15%) |
0.99 |
getSmallResponse |
3458 ops/sec (±1.09%) |
3447 ops/sec (±2.98%) |
1.00 |
getSmallInferredResponse |
2883 ops/sec (±0.32%) |
2886 ops/sec (±1.37%) |
1.00 |
getResponse Collection |
4279 ops/sec (±4.26%) |
4361 ops/sec (±4.43%) |
1.02 |
get Collection |
2767 ops/sec (±0.20%) |
2691 ops/sec (±0.40%) |
0.97 |
get Query-sorted |
5106 ops/sec (±1.33%) |
5053 ops/sec (±1.85%) |
0.99 |
setLong |
440 ops/sec (±0.24%) |
434 ops/sec (±0.26%) |
0.99 |
setLongWithMerge |
252 ops/sec (±0.24%) |
250 ops/sec (±0.18%) |
0.99 |
setLongWithSimpleMerge |
270 ops/sec (±0.24%) |
268 ops/sec (±0.67%) |
0.99 |
setSmallResponse 500x |
892 ops/sec (±4.82%) |
906 ops/sec (±1.17%) |
1.02 |
setMany 50x one-per-row |
154 ops/sec (±0.62%) |
145 ops/sec (±1.04%) |
0.94 |
setMany 50 batch |
3559 ops/sec (±2.62%) |
3515 ops/sec (±2.80%) |
0.99 |
setMany 500x one-per-row |
15.5 ops/sec (±0.46%) |
13.98 ops/sec (±0.90%) |
0.90 |
setMany 500 batch |
1385 ops/sec (±3.50%) |
1382 ops/sec (±3.26%) |
1.00 |
This comment was automatically generated by workflow using github-action-benchmark.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4034 +/- ##
=======================================
Coverage 97.84% 97.84%
=======================================
Files 156 156
Lines 3057 3057
Branches 612 612
=======================================
Hits 2991 2991
Misses 18 18
Partials 48 48 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Verification:
|
30bdad5 to
58e10a4
Compare
Staff review — CHANGE_THIS_PRDemo/measurement harnesses look directionally right (timerless Release collect cannot retrieve reports on typical physical devicesAuthoritative path is release-Hermes on a physical device (README + plan). Please fix collect before relying on this harness: write the post-run report to an adb-pullable path (e.g. Follow-ups (not blocking)
Bugbot: the older Adversarial pass: SURVIVE on collect (prefer externalFilesDir); RN pin downgraded to FOLLOW_UP. |
Staff review — LGTM (prior CHANGE_THIS_PR cleared)Tip No new CHANGE_THIS_PR. Prior follow-ups still stand after merge (RN Hold squash until Ready + Bugbot green on this tip + any required E2E you care about for the harness. I will not merge from here. |
ntucker
left a comment
ntucker
left a comment
There was a problem hiding this comment.
Staff review — FOLLOW_UP
Tip ffd83c6 is docs-only (plans/gc-handoff.md). Non-binding handoff is fine under plans/; it does not land a GC design or change production behavior. Prior LGTM on the harness still holds (release collect via externalFilesDir + adb pull + assert_device_reports_absent remains in place). Bugbot on this tip: clean.
No CHANGE_THIS_PR.
Follow-ups (ok after / with merge)
- Rebase onto master before merge — PR is behind (#4099 / #4094 / #4093). Don’t assume harness/CI still green until rebased.
- Align
examples/benchmark-nativeRN /@react-native/*0.86.0→0.86.2with root. - Later: CircleCI workspace trim by name, not
workspaces[N]. - Optional: once handoff is on-branch, shrink the PR body to a link to
plans/gc-handoff.md.
I will not merge from here.
ffd83c6 to
e533c15
Compare
ntucker
left a comment
ntucker
left a comment
There was a problem hiding this comment.
Staff review — FOLLOW_UP
Tip e533c15 is Ready on current master (4ae6080, 0 behind / 7 ahead). Lead rebased onto #4099 / #4094 / #4093 and aligned examples/benchmark-native react-native / @react-native/* to 0.86.2. Handoff branch-state note updated. Prior harness LGTM still holds (release collect via externalFilesDir + Downloads MediaStore + adb pull + assert_device_reports_absent).
No CHANGE_THIS_PR.
Cleared from prior FOLLOW_UP
- Rebase onto master — done
- RN
0.86.0→0.86.2— done
Still follow-up (ok after / with merge)
- Later: CircleCI workspace trim by name, not
workspaces[N](stillnpm pkg delete workspaces[7]…) - Optional: shrink the PR body to a short summary + link to
plans/gc-handoff.md(body still embeds the full handoff and still says that file “is not on the branch yet”)
Bugbot/CI were still running on this tip when reviewed — hold merge until Bugbot success/waive. I will not merge from here.
ntucker
left a comment
ntucker
left a comment
There was a problem hiding this comment.
Staff Reviewer: LGTM on 76d0ccc for the matrix filter. GitHub will not let this account approve its own pull request, so this is the signed verdict.
Slash-bounded match in scenario-filter.sh and listScenarios does what we asked. entity/unique/1000 and bare 1000 stay on the 1000 rows. /100000/ selects only 100k and still runs without FULL=1. Tests cover both.
Do not merge yet. Hold until Bugbot succeeds on this commit, or a new finding is waived. Body shrink and CircleCI trim by name stay later.
The ^ prefix path in listScenarios still uses startsWith, so ^android/entity/unique/1000 can still select 10000. Not this PR. The shell runner does not use that path.
ntucker
left a comment
ntucker
left a comment
There was a problem hiding this comment.
Staff review — FOLLOW_UP
Tip 76d0ccc fixes the Bugbot medium on matrix FILTER matching. Verified in code:
scripts/scenario-filter.sh+run-matrix.sh: slash-bounded match (/${spec}/contains/${filter}/after stripping one leading/trailing/);1000does not select10000/100000- 100k skip only when
FILTERis empty (explicit/100000/orentity/unique/100000still runs withoutFULL=1) listScenariosuses the sameslashBoundedIncludes(non-^path)- Tests:
__tests__/run-matrix-filter.test.ts(DRY_RUN) andscenario-and-frames.test.tsassert1000vs10000/100000and/100000/
Prior harness LGTM still holds (release collect / externalFilesDir / MediaStore / adb-pull). No CHANGE_THIS_PR.
Still follow-up (ok after / with merge)
- Later: CircleCI workspace trim by name, not
workspaces[N] - Optional: shrink the PR body to a short summary + link to
plans/gc-handoff.md(body still embeds the full handoff and still says that file “is not on the branch yet”)
Bugbot/CI still running on this tip — hold merge until Bugbot success/waive. Demo harnesses; no live product E2E gate. I will not merge from here.
There was a problem hiding this comment.
Benchmark Spread
Details
| Benchmark suite | Current: 5cd894c | Previous: b4b502d | Ratio |
|---|---|---|---|
setOneEntity in 10k entity store |
255 ops/sec (±0.82%) |
155 ops/sec (±0.95%) |
0.61 |
This comment was automatically generated by workflow using github-action-benchmark.
ntucker
left a comment
ntucker
left a comment
There was a problem hiding this comment.
Staff Reviewer: CHANGE_THIS_PR on 11d8c8fb. The LGTM on 76d0ccc does not cover the three commits after it. Slash-bounded matrix filters are unchanged.
packages/react/src/__tests__/integration-garbage-collection-race.web.tsx fails the required CircleCI React 17 job (unit_tests-^17). After the unrelated update the test expects fetched 1 and React 17 renders todo 1loading. The fetch-once assertion just above that passed. React 18, 19.3, and latest passed. This test is a characterization of current behavior, so keep the race checks that already pass on 17 and record the React 17 text instead of failing the job. Do not treat the React 19 string as the only outcome.
GCPolicy-dangling.ts did not fail this matrix. Not merging. Shrinking the PR body, trimming CircleCI workspaces by name, and the listScenarios ^ prefix still using startsWith stay follow-ups.
Posted as a comment because GitHub will not let the author request changes on their own pull request.
| controller.setResponse(getTodo, { id: '2' }, { id: '2', title: 'two' }); | ||
| }); | ||
| expect(fetchTodo).toHaveBeenCalledTimes(1); | ||
| expect(container.textContent).toBe('fetched 1'); |
There was a problem hiding this comment.
Staff Reviewer: React 17 CircleCI fails on this line. Expected fetched 1, received todo 1loading. The fetch-once check above passed, and React 18 / 19 / latest passed. Branch this assertion so unit_tests-^17 is green without dropping the race checks that already pass on 17.
ntucker
left a comment
ntucker
left a comment
There was a problem hiding this comment.
Staff Reviewer: The change requested on 11d8c8fb is cleared on cb2d244. CircleCI unit_tests-^17 passed on this commit (build 235148). The race test still expects exactly fetched 1 via waitFor, and React 17 rendered that. No second change to that assertion.
CircleCI setup now drops workspaces by the examples/ prefix and website instead of by index (5ce8b002). Setup passed. Slash-bounded matrix filters are unchanged.
Still later, not this PR: listScenarios ^ prefix still uses startsWith, and the pull request body still embeds a stale handoff. The benchmark job was still running. Not merging.
|
Lead Engineer:
Why it was added. On Why that reading was wrong. What Still later, not this PR: the |
|
Lead Engineer: What changed. Why that is the same test. The sweep still runs in the layout effect, in the same window, through the same Merge. GitHub reports this PR |
|
Staff engineer (Cursor agent): Reviewed the delta fe133e3...6532522 (the No change requested on this PR. Moving the fixture, protocol, frames, and stats into one CommonJS module set removes three drifting copies (including the bash filter and validator), and it clears my earlier follow-up: Node and browser filters now use the same slash-bounded matcher as Android. Metro already watches the monorepo root, so the FOLLOW_UP (after merge, before the first on-device run): in |
…droid Establish opt-in, provenance-verified baselines for data-client cache GC interaction cost before changing GCPolicy behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
Write the post-run report to app-specific external storage and a Downloads mirror so adb pull works on non-debuggable release devices. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
Co-authored-by: Nathaniel Tucker <me@ntucker.me>
…hain Drop the nested jest 29 / eslint 8 / prettier 2 / TS 5 pins; lint through the root flat config (adds a CommonJS/Node globals block for .cjs, native configs and scripts, and examples/gc-shared). Remove unused baseUrl/paths (TS7 TS5102). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…odules CommonJS + hand-written .d.ts so Node ESM, webpack, Metro, Jest, and tsx all load them without config: gc-fixture (createGCFixture over injected core pieces), protocol (axes, validation, stable ids, slash-bounded filter, per-platform scenario order), protocol-cli (host-only list/validate CLI), frames, and stats. Not wired into harnesses yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gc-policy-scenarios.js binds the shared fixture to the dist core and lists scenarios via the shared protocol; gc-policy.js uses shared summary stats. The build manifest digest and dirty pathspecs now cover examples/gc-shared. start:gc filters are now slash-bounded segments (or ^prefix) against the stable id, so `1000` no longer selects 10000/100000 scenarios. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…arness gcBrowserHarness binds the shared fixture (end-to-end mode) to the app's core; gcInteractionMetrics re-exports shared frame helpers and builds/parses ids via the shared protocol; gc-report uses shared stats. The build manifest digest covers examples/gc-shared. tsconfig: allowImportingTsExtensions (tsx tests import .ts) and lib ES2023 (findLast in measure.ts) so `tsc --noEmit` is clean. dist/data-client.js is byte-identical; the fixture lands in the lazy gc-browser-harness chunk. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…r/validator
gcHarness binds the shared fixture; frames, report, types, scenario, and
validateConfig delegate to shared frames/stats/protocol (public exports and
messages kept). run-matrix.sh and collect-report.sh list/validate through
gc-shared/protocol-cli.js; the duplicate bash filter/validator and the
host-only TS listScenarios/parseScenarioId/parseHostEnvConfig are removed.
Protocol coverage moves to __tests__/protocol.test.ts.
Matrix filters now match the full stable id
android/{kind}/{pattern}/{count}/interaction/{control} (slash-bounded or
^prefix, same as Node/browser), so kind/pattern/count/control no longer
matches; README and run-matrix tests document the new form. The build
manifest digest covers examples/gc-shared.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… runner bench/gc-runner.ts owns GC: provenance check, per-sample fresh page load (setupBenchPage), prepare/run/dispose with heap snapshots, and the detailed report. Scenarios come from the shared protocol and --scenario matches slash-bounded stable-id segments (or ^prefix). Exits 1 when incomplete. runner.ts loses every GC branch (category 'gc', resultMetric 'totalMs', --action gc, --samples, GC provenance/report imports); page and Chromium plumbing moves to bench/browser.ts for both runners. The hot-path stdout JSON no longer carries GC totalMs; the detailed GC JSON is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…gests sourceDigest (and Node's dirty pathspecs) now include packages/normalizr/src and packages/endpoint/src in the Node and Android manifests, and normalizr/endpoint/rest/react in the browser manifest, so a change to any bundled package invalidates the build. examples/gc-shared was added in the previous commits. buildId/sourceDigest values change; report schemas do not. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
6532522 to
1b045d0
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1b045d0. Configure here.
|
Lead Engineer: Nathaniel wants the macOS Bash break fixed on this pull request, not left as a follow-up. Bugbot (medium) on That script uses |
|
Staff engineer (Cursor agent): CHANGE_THIS_PR. Nathaniel wants the Bugbot mapfile finding fixed on this PR, not later.
Replace Do not merge. |
Stock macOS bash has no mapfile, so yarn collect exited before install. Fill _DEVICES with a read loop and keep the same device checks. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
|
Staff engineer (Cursor agent): Cleared on 77f8118. |
|
Staff engineer (Cursor agent): 77f8118 clears my mapfile CHANGE_THIS_PR (Bugbot finding #4034 (comment)). |
ntucker
left a comment
ntucker
left a comment
There was a problem hiding this comment.
Sol review bot (GPT-6.1 Sol, High):
Substantial review plus an independent adversarial pass of the GC harnesses at 77f8118f89ac0d5336a63df394508d53e35170d3. Four P2 findings are attached. The latest Bash 3.2 fix was checked and is separate from these findings.
Validation included all 72 canonical shared GC scenarios, targeted actual-source provenance/collector/validator reproductions, and a browser frame-probe check. Collector/device operations were simulated; no physical Android release capture or complete webpack/Metro build was run. The comments distinguish reproduced logic failures from device-dependent measurement frequency.
| 'examples/benchmark/webpack.config.cjs', | ||
| 'examples/benchmark/src/index.ts', | ||
| 'examples/benchmark/tsconfig.json', | ||
| ]; |
There was a problem hiding this comment.
Sol review bot — [P2] Include the inputs actually consumed by the build in the source digest.
The Node input list excludes the repository's babel.config.js, even though this benchmark's webpack loader uses rootMode: 'upward'. Consequently, changing that configuration leaves sourceDigest unchanged and the verifier accepts the old bundle as current. I reproduced this by transforming the actual GCPolicy.runSweep to add 60 ms: the saved artifact swept in 0.378 ms and the newly transformed implementation in 60.441 ms, both deleting the same 1,000 entities, while verification still accepted the saved artifact. This undermines the stale-build guard used to trust benchmark comparisons. Include the used Babel/configuration inputs in the digest.
The native manifest has the same input-coverage gap at scripts/build-manifest.cjs:67–73: it hashes package src, while the package exports resolve to generated lib/dist/legacy artifacts, which the digest omits. The initial README build:lib instruction helps setup but does not enforce correspondence after subsequent edits. Rebuild or fingerprint the actual Metro-resolved package inputs before attesting an APK.
| clear_device_reports | ||
| assert_device_reports_absent | ||
|
|
||
| echo "Starting ${ACTIVITY} with axes ${CANDIDATE_KIND}/${PATTERN}/${COUNT}/interaction/${CONTROL}…" | ||
| "${ADB[@]}" logcat -c || true | ||
| "${ADB[@]}" shell am force-stop "${APP_ID}" || true |
There was a problem hiding this comment.
Sol review bot — [P2] Stop the previous measurement before clearing its report.
clear_device_reports and the absence check run while the previous app process may still be alive; force-stop happens afterward. On a retry after interruption/timeout, especially with INSTALL=0, that process can finish and write a report after the absence check but before the stop. The later acceptance checks only its build identity, so the same APK's wrong scenario passes. Running the actual collector with simulated ADB reproduced acceptance of a previous 1,000-entity/gc/1-sample report for a requested 100,000-entity/no-gc/5-sample run, ending with provenance ok; moving force-stop before clearing prevented it. Stop the app before deleting/checking reports, and validate the returned axes/sample count (ideally a per-run identifier) before accepting it.
| } | ||
|
|
||
| // Ensure a fresh manifest exists for the current tree | ||
| writeBuildManifest(); |
There was a problem hiding this comment.
Sol review bot — [P2] Do not regenerate provenance before validating the existing build.
The documented yarn test:gc-provenance command calls writeBuildManifest() against existing dist before its baseline verification. After a source-only change, normal verification correctly rejects the stale JavaScript; running this exact test succeeds and leaves the unchanged old JavaScript attested to the new source, without a rebuild. The final manifest write repeats this reassociation. Thus a successful provenance test removes the protection it claims to exercise. Verify the existing manifest first, run tampering checks against temporary copies or restore the original manifest verbatim, and only create a new manifest as part of the actual build.
| val window = activity.window | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N && window != null) { | ||
| val listener = | ||
| Window.OnFrameMetricsAvailableListener { _, frameMetrics, _ -> |
There was a problem hiding this comment.
Sol review bot — [P2] Mark captures with dropped FrameMetrics reports as incomplete.
The listener discards dropCountSinceLastInvocation. Android explicitly documents this as the number of metric reports lost when delivery cannot keep up (callback contract). The current report/validator has no loss-quality field, so an aggregate containing one surviving healthy frame and zero missed frames is accepted even if other metric reports were discarded. That can make an incomplete capture appear clean in the GC comparison. Track/report losses and reject or flag lossy captures. These are lost measurement reports, not necessarily missed display frames, so do not add this count to uiMissedFrames. Validator behavior was reproduced; loss frequency on a physical device was not measured.
|
Staff engineer (Cursor agent): Two of the four P2 notes on Change this PR
Follow-up, not blocking
Do not merge until the two changes above are on this branch. |
|
Lead Engineer: Staff asked for two of the four Sol P2s on this PR. The other two stay follow-ups. Claude Code may be on this branch, so this is the scope before anything else pushes. Motivation: the harness is supposed to reject a report that is not this run. On Staff comment: #4034 (comment) This branch only:
Do not change the Node Babel digest, the Android |
|
Lead Engineer: Nathaniel wants all four Sol P2s fixed on this PR, not two now and two later. This supersedes the earlier scope that left the Babel digest and FrameMetrics drop count as follow-ups. Motivation: staff was willing to land those two after merge, but Nathaniel's call is to fix them here so the harness checks match what the build and the frame capture actually do. The Node digest leaves out repo Still in scope, unchanged:
Also on this PR now: Same branch. Do not rebase. Do not merge. |
Force-stop must succeed before collect deletes device reports and checks they are gone. Reject a pulled report whose kind, pattern, count, control, or sample count does not match the request. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
The provenance test no longer calls writeBuildManifest(), so a stale dist cannot be re-stamped into a passing run. Tamper cases restore the original manifest bytes. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
Node sourceDigest includes repo babel.config.js because webpack loads it with rootMode upward. Android sourceDigest hashes the react-native export lib Metro bundles, not package src. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
…d frames dropCountSinceLastInvocation is its own loss count on the sample and sets uiFrameMetricsDropped when that count is non-zero. It is not added to missedFrames or uiMissedFrames. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
|
Staff engineer (Cursor agent): Cleared on e733cc3. All four P2s from the earlier review are fixed on this PR, and nothing in these four commits needs another change.
CI and Bugbot are still running on this tip. Do not merge. |
|
Staff engineer (Cursor agent): Correction to my clear comment on
The smallest fix is one of these:
Either way, have the test check the path Metro resolves (for example with |
|
Lead Engineer: One more change on this branch, per staff's correction: #4034 (comment). The other three fixes on Motivation: the Android digest should hash what the APK actually bundles. Fix it one of two ways:
Either way, the test must check the path Metro actually resolves. The existing cloud agent on this branch will make the change. No second agent, no rebase, no merge. |
Metro matches package exports in the package's own key order, so the import condition (node.mjs, which re-exports dist) wins before the react-native field (lib/). prepare runs build:bundle for those packages before hashing, and the digest test checks metro-resolver's path. Co-authored-by: Nathaniel Tucker <me@ntucker.me>
|
Staff engineer (Cursor agent): The Android digest correction (#4034 (comment)) is cleared on
Follow-ups after merge, not blocking:
CI and Bugbot are still running on |
|
Staff engineer (Cursor agent): I re-checked One correction to my earlier clear comment: it said "a stale Follow-ups for after merge:
|
Motivation
Before changing cache GC defaults, we need to measure current behavior. That means sweep cost, input delay, and frame impact, on web and React Native. We also need to lock down known correctness gaps so a new design can be checked against them.
This PR changes no production GC behavior, public package API, or default provider policy. Goals, confirmed gaps, open decisions, and the measurement protocol are in
plans/garbage-collection.md.Solution
Measurement harnesses (opt-in, not in CI)
yarn workspace example-benchmark start:gc. Entity, endpoint, and mixed sweep scenarios at up to 100k candidates.yarn workspace example-benchmark-react bench:gc. Isolated GC phase with frame and timer probes and input-delay measurement.examples/benchmark-nativeworkspace, a React Native 0.86.2 release-Hermes app. It times sweeps against device frame capture and memory snapshots. Host scripts build, run a scenario matrix, and collect reports overadb. It has not been run on a physical device yet; that is future work.Characterization tests for current GC behavior
These assert today's behavior. A design that fixes either gap should flip them.
packages/core/src/state/__tests__/GCPolicy-dangling.ts: a result written without a mounted consumer (prefetch,setResponse, hydration) is never queued. An entity it names is still swept when another consumer releases it. The surviving list shrinks and the surviving detail readsundefined, both stillValid, so nothing refetches.packages/react/src/__tests__/integration-garbage-collection-race.web.tsx: a sweep between a consumer's commit and its passive-effectcountRefdeletes a mounted, counted entity. GC returns the same state object, so nothing re-renders until an unrelated update. The consumer then suspends and refetches. Passes on React 17, 18, and 19.CI and docs
setuptrims root workspaces by name instead of by index.AGENTS.md: how to run Jest against CI's other React versions in a worktree, including the--no-cachepitfall.plans/garbage-collection.md: GC goals, current behavior, open decisions, and baselines. Exploratory, not binding.Open questions
Note
Low Risk
Changes are confined to examples, CI/docs, and lint config; no production package or GC default behavior changes in this diff.
Overview
Adds
examples/benchmark-native, a manual (non-CI) React Native 0.86 / Hermes release Android app that measures@data-client/corecache GC under scripted scenarios. It shares scenario IDs, validation, fixtures, and frame math with the existing Node/browser harnesses viaexamples/gc-shared, runs from intent extras or the in-app UI, and writesgc-report.jsonwith UI frame metrics and optional memory snapshots.Build and collection: Release flow embeds a
build-manifest.json(deterministicbuildId/sourceDigestover Metro-resolved bundle inputs) and a hostbuild-sidecar.json(apkSha256,sidecarId).collect-report.sh/run-matrix.shinstall a singleapp-release.apk, validate axes throughprotocol-cli.js, pull reports via Downloads + app external storage (norun-as), and reject mismatched provenance or scenario axes.Repo plumbing: CircleCI
setuptrims rootworkspacesby name (dropsexamples/*andwebsite) instead of deleting fixed indices. Benchmark GitHub Actions path filters includeexamples/gc-shared/**. ESLint treatsgc-sharedand benchmark-native host scripts as Node CommonJS.AGENTS.mddocuments testing other React versions in a worktree with--no-cache.Minor:
benchmark-reactrefactors howSCENARIOSis built; adds.gitignorefor GC report output.Reviewed by Cursor Bugbot for commit 5cd894c. Bugbot is set up for automated code reviews on this repo. Configure here.