perf(android): keep the automation helper warm across fill and scroll - #1974
Open
thymikee wants to merge 3 commits into
Open
perf(android): keep the automation helper warm across fill and scroll#1974thymikee wants to merge 3 commits into
thymikee wants to merge 3 commits into
Conversation
Android permits one UiAutomation owner, so a command-scoped helper call stops the instrumentation session when it finishes and the next call pays a fresh `am instrument` start plus the UiAutomation connect wait. `fill` reads the live hierarchy four times per attempt (the pre-action target read plus the 0/150/350 ms settling samples) and every one of those reads was command-scoped, so a single fill ran four instrumentation lifecycles — seven on retry — and left `scroll` without a session for its viewport read. Thread the existing `daemon-session` scope, which app-backed sessions already select for snapshots and whose release session teardown already owns, through fill's captures and the gesture viewport read. The viewport read may now warm the session so it and the gesture that follows share one instrumentation instead of starting two. Snapshot capture and the viewport read build their capture options through one path, because the session identity is derived from them: two builders would restart each other's session instead of sharing it. Session teardown also stops force-stopping the runtime once the helper both acknowledged `quit` and its process exit was observed — that pair is the release evidence. Forced, timed-out, and aborted teardowns still force-stop, so a daemon that dies without sending `quit` cannot leave the helper squatting UiAutomation. The settling samples still capture separately; only the session is shared.
REVIEW.md F1. Gating the teardown force-stop on confirmed release also changed what `stopAndroidSnapshotHelperSession` returning `true` means to its callers. `retireAndroidSnapshotHelperAfterContentFailure` reads that return as "the session stop was the runtime reset" and skips `resetAndroidSnapshotHelperRuntime` when it is true, so a helper whose output failed content validation three times over a daemon session was retired with no `am force-stop` at all, while the one-shot arm of the same recovery still reset the runtime. The session stop now takes `resetRuntime`, and the retirement passes it. Content failure is a recovery path, not a clean release: the helper answered with output we could not trust, so the next capture must meet a runtime that was reset, and "it quit politely" is not a reason to leave a suspect process owning it. Genuine session close still skips the round trip, which is the optimization this branch exists for. The pre-existing test "content failure retirement does not layer a second reset over a persistent session stop" rested on a premise this branch had already made false — stopping a live session was no longer the runtime reset. It is renamed to "makes the session stop reset the runtime instead of layering a second one" and asserts the requirement it now depends on, so its title states what it enforces. The reviewer's probe returns as a regression test in `snapshot.test.ts`. REVIEW.md F2. `observeAndroidSnapshotHelperProcessExit` returns an observation that knows whether the end it saw is release evidence: the process must have been alive when the teardown started watching and must then exit with code 0 and no terminating signal. A signal, a non-zero code, and a host child that was already gone before `quit` was sent all mean the transport died — and in the pre-died case the acknowledgement is positive evidence that the device-side helper OUTLIVED its host through the open forward. None of them confirm release now. What is left is declared at the gate: host-side exit codes are only as strong as adb's exit forwarding, and a device without shell protocol v2 can report 0 for an instrumentation that did not finish. REVIEW.md F3. `&& graceful.exited` survived deletion against all 423 tests. Two named tests now fail without it, both cheap because the stricter evidence above makes acknowledged-but-not-released reachable without waiting out the 11 s graceful-exit timeout. Gates: `pnpm vitest run src/platforms/android` (44 files, 423 tests) and with `src/core/interactors` (426 tests), `pnpm typecheck`, `pnpm lint`, `pnpm format`, `pnpm check:fallow --base origin/main` (24 changed files, clean). No emulator is attached in this worktree, so the teardown-path force-stop counts were not re-observed live.
Size Report
npm unpacked components
Startup median (7 runs, lower is better):
Top changed chunks:
Top changed packed files
|
This branch extracted the helper-session, retirement, and touch-helper cases
out of src/platforms/android/__tests__/snapshot.test.ts into sibling files,
taking it from 1,658 to 1,495 lines. The test-file size ratchet fails an
un-banked shrink ("lower its pin in this PR so the ratchet keeps the gain"),
so bank it. The pin only moves down; nothing grew into it.
Member
Author
|
The lifecycle and regression coverage look sound, but this is not ready at |
Member
Author
|
Two blockers remain at this head:
Current main also overlaps the fill files, so please rebase and revalidate after restructuring. The lifecycle tests, device evidence, and exact-head CI are otherwise strong. |
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
1 participant
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.
Closes the largest verified performance item from the 2026-08 architecture audit (Android hot path, ~10–20x on fill).
Summary
Fill ran 4 full instrumentation lifecycles per invocation (7 on retry) and scroll two one-shot runs, because fill's verification captures and scroll's viewport read never passed the daemon-session
helperSessionScopethat already existed for snapshots — every capture tore down the helper (graceful quit +am force-stop+forward --remove) and the next spawned a fresham instrumentwith a up-to-10s UiAutomation wait.fill-verification.ts) and scroll's viewport read (touch-helper.ts) now share the session's scope; scope derivation has one owner (androidHelperSessionScopein the interactor) and capture options one builder, so snapshot/fill/scroll cannot derive divergent session identities.am force-stoponly when release is confirmed (graceful.acknowledged && graceful.exited); forced/timeout/abort/failed-start paths still force-stop.Validation
2febe31bf; F3 fenced; F2 documented at the gate.Pixel_7_review, backend proof viaandroidSnapshot.backend: android-helper, helper starts counted device-side via logcat): fill 2910→1673ms wall, helper starts 3→0; scroll persistent-session on every run, helper starts 2→0; force-stop round trip gone fromclose.check:affected(272 files / 1821 tests), typecheck, lint, layering, fallow, production-exports all green.Residual risk
API < 34 cache flush uses a
setServiceInfo()re-apply rather thanUiAutomation.clearCache()(declared residue of #1832) — shared-session settling observation relies on it; analysis in RESULT.md.