Skip to content

perf(android): avoid animation stalls in capture and viewport reads - #3790

Merged
thymikee merged 5 commits into
mainfrom
t3/investigate-speed-issue
Oct 18, 2026
Merged

thymikee merged 5 commits into
mainfrom
t3/investigate-speed-issue

Conversation

@thymikee

@thymikee thymikee commented Oct 17, 2026 •

Copy link
Copy Markdown
Member

Summary

Follow-up to #3558: Android capture and viewport reads stalled on busy apps.

  • Batch up to eight sibling accessibility reads, preserving order, limits, freshness, and cleanup.
  • Share window preparation between captures and viewport reads. Retain the 100 ms quiet interval; reduce the viewport's maximum idle wait from two seconds to 500 ms. Snapshot settings remain unchanged.
  • Unify node/window bounds serialization. Make the cleanup concurrency test deterministic and document optional keyboard headers.

Nine helper files changed. Scope expanded to viewport reads and serialization at the user's request.

Validation

Head 7a74fb8e354a5606385c8e6d571018e242251f7d, rebased onto c966724beafd8be5106337a4d95018278666237c; range-diff preserves the original commits.

  • pnpm build:android and full-repository pnpm format: passed.
  • pnpm check:affected --run --head HEAD: passed at the named SHA, receipt run-kMrAQk, exactCommit: true. Helper builds are GitHub-authoritative; full replay is manual-dispatch. Both skipped locally.
  • API 35 viewport: continuous events 2018 → 510 ms; keyboard-resized window 2013 → 515 ms, identical responses. Static reads remained fast.
  • Immediate tap-then-scroll verification passed three times; swipes stayed above the appearing keyboard and the app scrolled.
  • JVM suite passed; an isolated early-return mutation was detected by the cleanup test. Independent review passed.

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.

@thymikee

Copy link
Copy Markdown
Member Author

Validation evidence for 2ecc9523189c55c8d41a2bd207043d1aed997004 (base 4a8abed7b969cffc47eb2618e8dfb804116ff85b).

pnpm check:affected --run --head 2ecc9523189c55c8d41a2bd207043d1aed997004 receipt run-dMJYxT:

{
  "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: pnpm build:android passed. Full-repository pnpm format completed. Independent review found an API 23 functional-interface issue; it was fixed, and the second review had no findings.

Live device: emulator-5554, Android API 35. Six captures per revision and condition, alternating APK installs on the same device. Same capture command:

adb -s emulator-5554 shell am instrument -w -e waitForIdleTimeoutMs 0 com.callstack.agentdevice.snapshothelper/.SnapshotInstrumentation

Fixture: 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 elapsedMs samples:

animated base: 2730, 1798, 4058, 2065, 2975, 3085
animated head: 831, 960, 679, 713, 467, 758
static base: 96, 119, 123, 140, 99, 102
static head: 84, 68, 73, 86, 83, 77

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 maxNodes=50 and maxDepth=3 captures matched and reported truncation.

The source-backed CLI also passed snapshot -i --json and wait 'label="row-5-0"' 15000 with animation enabled. Snapshot metadata: backend=android-helper, helperVersion=0.21.29-dev, helperTransport=persistent-session, nodeCount=546, elapsedMs=620, no missing roots. Sessions and the verification daemon were closed.

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.

@github-actions

github-actions Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.39 MB 5.39 MB -81 B
Package (unpacked) 5.39 MB 5.39 MB -81 B
Package (download) 1.62 MB 1.63 MB +2.7 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 36.4 ms 35.0 ms -1.4 ms
CLI --help 67.9 ms 66.7 ms -1.2 ms

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@thymikee thymikee changed the title perf(android): batch snapshot reads on animating apps perf(android): avoid animation stalls in capture and viewport reads Oct 17, 2026
@thymikee

Copy link
Copy Markdown
Member Author

Follow-up validation at ab8474c68f2a85c5d32bda682f06e0a1dd2910ac, compared with the earlier PR head 2ecc9523189c55c8d41a2bd207043d1aed997004.

Confirmed changes:

  • Removed the global accessibility idle wait from viewport geometry reads. Both transports still use the same interactive-window readiness check. Snapshot idle policy and freshness remain unchanged.
  • Unified node/window bounds serialization in one direct numeric writer.

Warm persistent-session viewport samples on API 35, in milliseconds (four reads per condition):

base-repeat, events=True, keyboard=False: 2028.3, 2010.31, 2023.9, 2009.91
base-repeat, events=False, keyboard=False: 99.45, 2.78, 5.39, 10.13
base-repeat, events=True, keyboard=True: 2007.85, 2015.9, 2017.19, 2011.01
head-repeat, events=True, keyboard=False: 3.26, 11.31, 1.85, 2.07
head-repeat, events=False, keyboard=False: 2.31, 1.41, 1.55, 1.72
head-repeat, events=True, keyboard=True: 10.61, 2.82, 2.09, 9.9

The fixture emits TYPE_WINDOW_CONTENT_CHANGED every 30 ms. Each condition was prepared before timing. All 12 viewport responses matched base/head exactly, including the keyboard-resized application frame. The static control remained fast. One-shot viewport also succeeded.

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 String.format template and omits its additional XML escape scan, so this is a conservative isolated formatter comparison. It does not establish an end-to-end snapshot speedup.

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: snapshot -i --json reported android-helper, helper 0.21.29-dev. With the keyboard visible at y=1517, scroll down --pixels 300 used y=975→675, reported movement=moved, advanced the app marker 280→560, and kept the keyboard visible. This proves actual dispatch and app movement, rather than only a successful helper response.

JVM helper suite passed. In a disposable checkout, changing the left bound to Math.abs(left) caused the new wire-format test to fail on [-40,0][1080,2400] as intended. Independent review found no regressions. The disposable checkout, temporary fixture, benchmark artifact, verification session, and verification daemon were cleaned up.

Exact-head affected-check receipt run-Wb2KbU:

{
  "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.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread android/snapshot-helper/README.md Outdated
@thymikee

Copy link
Copy Markdown
Member Author

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.

@thymikee
thymikee force-pushed the t3/investigate-speed-issue branch from ab8474c to 7a74fb8 Compare October 18, 2026 08:36
@thymikee

Copy link
Copy Markdown
Member Author

Review update at 7a74fb8e354a5606385c8e6d571018e242251f7d, rebased onto c966724beafd8be5106337a4d95018278666237c. Range-diff shows the three original commits replayed unchanged, followed by the review fixes.

  • Fixed the cleanup test: it observes the closer blocked on the held reader before releasing it. An isolated early-return mutation failed at that assertion.
  • Documented the four optional keyboard headers and their absence semantics.

The optional immediate-keyboard concern did reproduce: on the no-idle version, press 'label="Probe input"' immediately followed by scroll down --pixels 900 planned y=1653→753, while the appearing keyboard top was y=1517. There was no intermediate capture or artificial delay. Although the app scrolled in that run, that plan did not stay above the appearing keyboard.

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. android-helper reported helper 0.21.29-dev.

The final viewport timings supersede the earlier 4–6 ms no-settle figures:

  • Continuous accessibility events: original 2018 ms → final 510 ms mean.
  • Keyboard-resized window: original 2013 ms → final 515 ms mean.
  • Static final reads: 2.81 ms mean.
  • All 12 viewport responses matched the original baseline.

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.

pnpm install --frozen-lockfile, pnpm build, pnpm build:android (including JVM suite), full-repository pnpm format, and independent review passed. The isolated mutation checkout, verification fixture, session, and daemon were cleaned up.

Affected-check receipt run-kMrAQk (CI-owned/manual Android checks are explicitly skipped locally):

{
  "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.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +63 to +65
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Suggested change
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.

@thymikee

Copy link
Copy Markdown
Member Author

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 waitsForReadersBeforeReleasingTheParent: the closer could show WAITING briefly during thread startup, before it calls close(). That looks implausible in practice, but I did not run it to confirm. The live API 35 timings and the keyboard tap-then-scroll run are author-reported. The original RN sheet-dialog repro from #3558 and API 23-32 devices are not re-verified, as the PR body says.

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 -e waitForIdleTimeoutMs/QuietMs. The host defaults match, so this is wording only. Please drop "the same ... used by snapshots", or say that viewport ignores those arguments. The thread on the test's WAITING check (#3790 (comment), P2) is fixed at this head, so you can resolve it.

Nothing blocks the merge. I'd fix the README wording first.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 18, 2026
@thymikee
thymikee merged commit cf42352 into main Oct 18, 2026
16 checks passed
@thymikee
thymikee deleted the t3/investigate-speed-issue branch October 18, 2026 10:05
@github-actions

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-18 10:05 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant