Repository navigation
perf(android): avoid animation stalls in capture and viewport reads - #3790
Conversation
|
Validation evidence for
{
"status": "passed",
"exitCode": 0,
"exactCommit": true,
"checkoutChanged": false,
"refs": {
"base": "origin/main",
"head": "2ecc9523189c55c8d41a2bd207043d1aed997004",
"resolvedBase": "4a8abed7b969cffc47eb2618e8dfb804116ff85b",
"resolvedHead": "2ecc9523189c55c8d41a2bd207043d1aed997004",
"mergeBase": "4a8abed7b969cffc47eb2618e8dfb804116ff85b"
},
"results": [
{
"id": "android-helpers",
"status": "skipped",
"reason": "GitHub-authoritative (jobs: Android / Smoke Tests)"
},
{
"id": "replay-android",
"status": "skipped",
"reason": "parked, workflow_dispatch only (Replay Manual / Android Full Emulator Suite)"
}
]
}Manual native build/JVM suite: Live device: adb -s emulator-5554 shell am instrument -w -e waitForIdleTimeoutMs 0 com.callstack.agentdevice.snapshothelper/.SnapshotInstrumentationFixture: a native dialog containing 100 rows of four text nodes, an opacity loop, and 12 ms of main-thread work per Choreographer frame. The static control disables that loop/work without changing the view tree. This isolates the Android traversal mechanism; it is not the reporter’s RN/TrueSheet application. Helper All 24 full captures had identical app subtrees, including child order and bounds (window-global root indexes were excluded because SystemUI windows can appear/disappear). Animated full captures returned 546 total nodes. On both revisions, additional The source-backed CLI also passed CI native/smoke results remain separate; the full manual-dispatch replay suite was not run. API 23 compatibility was reviewed and compiled, not exercised on an API 23 device. |
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
All reported issues were addressed across 5 files
You’re at about 98% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Follow-up validation at Confirmed changes:
Warm persistent-session viewport samples on API 35, in milliseconds (four reads per condition): The fixture emits Android bounds-writing microbenchmark, 100,000 writes per sample, six samples per path: median 133.8 → 6.9 ms; process allocations 148–152 MB → 0–3.3 MB. The old comparison uses the same numeric Alternating base/head APKs, 20 full static captures per revision: mean helper time 35.45 → 36.60 ms, unchanged within noise. All 40 dialog XML hashes matched. No claim of faster full snapshots from the serialization change. Live source CLI: JVM helper suite passed. In a disposable checkout, changing the left bound to Exact-head affected-check receipt {
"status": "passed",
"exitCode": 0,
"exactCommit": true,
"checkoutChanged": false,
"refs": {
"base": "origin/main",
"head": "ab8474c68f2a85c5d32bda682f06e0a1dd2910ac",
"resolvedBase": "4a8abed7b969cffc47eb2618e8dfb804116ff85b",
"resolvedHead": "ab8474c68f2a85c5d32bda682f06e0a1dd2910ac",
"mergeBase": "4a8abed7b969cffc47eb2618e8dfb804116ff85b"
},
"results": [
{
"id": "android-helpers",
"status": "skipped",
"reason": "GitHub-authoritative (jobs: Android / Smoke Tests)"
},
{
"id": "replay-android",
"status": "skipped",
"reason": "parked, workflow_dispatch only (Replay Manual / Android Full Emulator Suite)"
}
]
}The selected Android helper gate remains GitHub-authoritative. The full manual-dispatch Android replay suite was not run. CI results are separate from local validation. |
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
You’re at about 98% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Thanks for this. I reviewed ab8474c and found no problems in the production code that must change before merge. All 16 checks pass, including the Android / Smoke Tests job that builds the helper and runs the JVM suite this diff touches. I did not run the device benchmarks or the JVM suite myself, so the live numbers come from your comments. The animated-capture evidence is from 2ecc952, and the batching code is byte-identical at ab8474c, so it carries over. I did not verify that concurrent getChild calls on one parent node are safe on API 23-32 devices, and the original RN/TrueSheet screen from the report is still unconfirmed. Not blocking, and you can take or leave these: removing waitForIdle(100, 2000) in GestureViewportReader.java also removed an implicit 100 ms settle, so a swipe planned right after a tap that opens the keyboard may read windows before the input method window is listed and not clip above it (one live tap-into-field then immediate scroll, checking that the planned y stays above the keyboard top, would settle it); the bounds-writer change in AccessibilityTreeXml.java shows no end-to-end snapshot gain in your benchmark; and the batch-driven loop in appendNode has no JVM test because AccessibilityNodeInfo cannot be faked there, so a Reader-injected traversal seam would make it testable. One open review thread still needs a fix before human review: the Cubic P2 on the closer-thread latch in AccessibilityReadBatchTest still applies (#3790 (comment)). It is test-only, so making that latch deterministic is enough. The Cubic P3 on the README keyboard fields (#3790 (comment)) also still applies but does not block. Nothing else blocks this PR. |
ab8474c to
7a74fb8
Compare
|
Review update at
The optional immediate-keyboard concern did reproduce: on the no-idle version, I restored the short settle and unified window preparation. Viewport reads now share the snapshot defaults (100 ms quiet interval, 500 ms maximum); snapshot overrides and behavior are preserved. Three runs on the final head, each starting with the keyboard absent and issuing the same immediate press/scroll sequence, planned y=1275→375, kept the keyboard visible at y=1517, and advanced the app marker from 0 to 881. The final viewport timings supersede the earlier 4–6 ms no-settle figures:
The bounds writer remains a small allocation/CPU improvement; I am not claiming faster complete snapshots from it. I did not add a traversal seam solely for tests, consistent with the repository rule against test-only dependency injection. API 23–32 device behavior and the original RN/TrueSheet screen remain unverified.
Affected-check receipt {
"status": "passed",
"exactCommit": true,
"checkoutChanged": false,
"refs": {
"base": "origin/main",
"head": "HEAD",
"resolvedBase": "c966724beafd8be5106337a4d95018278666237c",
"resolvedHead": "7a74fb8e354a5606385c8e6d571018e242251f7d",
"mergeBase": "c966724beafd8be5106337a4d95018278666237c"
},
"results": [
{
"id": "android-helpers",
"status": "skipped",
"reason": "GitHub-authoritative (jobs: Android / Smoke Tests)"
},
{
"id": "replay-android",
"status": "skipped",
"reason": "parked, workflow_dispatch only (Replay Manual / Android Full Emulator Suite)"
}
]
}CI is rerunning at the new head; old green checks and performance evidence remain attributed to their original SHAs. |
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
You’re at about 99% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="android/snapshot-helper/README.md">
<violation number="1" location="android/snapshot-helper/README.md:63">
P3: This says viewport uses the same idle wait as snapshots, but viewport reads always call `prepare(automation)` with the 100 ms/500 ms defaults while snapshots honor the supplied wait arguments. Describe the viewport values as fixed defaults to avoid implying that `-e waitForIdleTimeoutMs` or `-e waitForIdleQuietMs` configures both modes.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| The `viewport` mode reads application and keyboard bounds after the same best-effort idle wait | ||
| used by snapshots: a 100 ms quiet interval with a 500 ms maximum wait. This allows short window | ||
| transitions, including keyboard appearance, to settle without waiting indefinitely on live content. |
There was a problem hiding this comment.
P3: This says viewport uses the same idle wait as snapshots, but viewport reads always call prepare(automation) with the 100 ms/500 ms defaults while snapshots honor the supplied wait arguments. Describe the viewport values as fixed defaults to avoid implying that -e waitForIdleTimeoutMs or -e waitForIdleQuietMs configures both modes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At android/snapshot-helper/README.md, line 63:
<comment>This says viewport uses the same idle wait as snapshots, but viewport reads always call `prepare(automation)` with the 100 ms/500 ms defaults while snapshots honor the supplied wait arguments. Describe the viewport values as fixed defaults to avoid implying that `-e waitForIdleTimeoutMs` or `-e waitForIdleQuietMs` configures both modes.</comment>
<file context>
@@ -60,9 +60,10 @@ results are not reused across captures.
-The `viewport` mode reads application and keyboard bounds while content updates continue; it does
-not wait for the accessibility event stream to become idle. Snapshot mode uses the idle wait
-configured above.
+The `viewport` mode reads application and keyboard bounds after the same best-effort idle wait
+used by snapshots: a 100 ms quiet interval with a 500 ms maximum wait. This allows short window
+transitions, including keyboard appearance, to settle without waiting indefinitely on live content.
</file context>
| The `viewport` mode reads application and keyboard bounds after the same best-effort idle wait | |
| used by snapshots: a 100 ms quiet interval with a 500 ms maximum wait. This allows short window | |
| transitions, including keyboard appearance, to settle without waiting indefinitely on live content. | |
| The `viewport` mode reads application and keyboard bounds after a best-effort idle wait of 100 ms quiet and 500 ms maximum. These viewport values are fixed; snapshot mode uses the idle wait configured above. |
|
The code in 7a74fb8 looks good to me. The earlier findings from ab8474c are addressed, and I have no new blocking findings. All 16 checks pass at 7a74fb8. The helper-APK builds run in CI and cover the changed snapshot-helper sources. I did not run the JVM suite or the early-return mutation. I judged the test's determinism by reading the code. One small race remains in One thread still applies: the README wording on viewport idle wait (#3790 (comment), P3). README.md:63-66 says viewport uses "the same" idle wait as snapshots. But GestureViewportReader.java:67 calls prepare(automation) with fixed 100/500 ms defaults, while SnapshotInstrumentation.java:358 honors Nothing blocks the merge. I'd fix the README wording first. |
|
Summary
Follow-up to #3558: Android capture and viewport reads stalled on busy apps.
Nine helper files changed. Scope expanded to viewport reads and serialization at the user's request.
Validation
Head
7a74fb8e354a5606385c8e6d571018e242251f7d, rebased ontoc966724beafd8be5106337a4d95018278666237c; range-diff preserves the original commits.pnpm build:androidand full-repositorypnpm format: passed.pnpm check:affected --run --head HEAD: passed at the named SHA, receiptrun-kMrAQk,exactCommit: true. Helper builds are GitHub-authoritative; full replay is manual-dispatch. Both skipped locally.Earlier capture evidence at
2ecc952318: 2.79 → 0.74 s. Bounds writer CPU/allocation improved in isolation; full snapshot timing was unchanged within noise. Original RN screen and API 23–32 devices remain unverified. CI results are separate.