refactor(ios): normalize snapshot geometry in one pass after acquisition (#2661) - #2669
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
Reviewed at dccb40f. The frontier still reads pre-normalization geometry, so a regular When The issue's acceptance criteria list four live results — portrait Not blocking: the ADR and the comment near RunnerTests+Snapshot.swift#L219 say a duplicate's children attach to "the first node it duplicated," but the code attaches them to The post-pass itself is small, about 70 lines in I didn't run the runner on a simulator, so the frontier divergence is inferred from the code, not observed; I can't say whether the real landscape keyboard tree has an owned Cell or scroll container that a native-space rect would actually project out, or whether it only shifts presented depth, and I didn't check whether any existing runner keyboard fixture already exercises the recursive walk with Repo Guards failed in Before this can merge, the frontier needs to stop reading unnormalized rects mid-walk, and the portrait, landscape, and benchmark evidence above needs to come from a live run against this head. |
|
Overlap with #2664 (opened earlier today, 25 files, CI green, waiting on one live check). #2664 already moves the rotation table into the package as Proposed order: #2664 lands first, then this rebases onto main and becomes the behaviour layer only:
That also takes the move out of your diff, which should bring it under the 1,000 budget without a split. Separately, Repo Guards is red on this head ( |
|
Answers to the two questions, plus one correction to my earlier note. Correction first. #2664 was trimmed to the package move only (head Q1 — overlap: retarget onto #2664's branch. Q2 — the frontier: drop the fold-based frontier from the walk and present-cut on the normalized array, the way the private-AX tier already does. "Normalize-then-frontier" is #2653's threading with a different name: it keeps geometry computed inside the walk, which is the coupling #2661 exists to remove. The frontier's legitimate job is a work bound, and a work bound must not consult presentation — issue #2661 step 1 says exactly this: keep a raw depth plus node cap ( The byte-identity rule you should hold yourself to:
One thing to watch: the frontier also feeds the scroll hints ( Repo Guards: agreed it is the |
|
That settles the design question for dccb40f: the rotation-table move belongs to #2664, and this PR rewrites onto the API at 321aef2. The frontier plan (drop the fold-based frontier, keep a raw depth and node cap, and present-cut on the normalized array) matches the open finding. Before this is ready, the next head needs that rewrite with the #2661 step 1 measurement, the live portrait, landscape and benchmark runs from the earlier comment, and a rebase, because the PR now conflicts with its new base. |
ae023d5 to
47b3aa1
Compare
|
Rewritten onto the merged move (this head rebases onto main after #2664 merged as Q1 — done. #2664's move is gone from this PR; I now use Q2 — done, dropped the fold frontier. No geometry read remains in the walk. A regular capture bounds itself by raw traversal depth (nil) plus a runaway node cap; the fold + presented-depth cut happen in Proven in CI/locus, head Live runs — need a decision before I burn a baseline build. The deterministic suite can't settle your byte-identity rule because the frontier also fed the presented-depth boundary scroll hints, so a portrait
Repo Guards is a fresh run on this head (not the tmpdir-flake run); I'll rerun it if it goes red again. |
|
Checked Your "still owed" list is the merge gate, same one #2664 was held to: portrait |
47b3aa1 to
80ac6ae
Compare
|
Reviewed 80ac6ae, following up on dccb40f. Two things block this. Regular iOS snapshots now take a new route: acquisition publishes reported frames and one normalized pass follows, so regular The new Not blocking, take or leave: a CHANGELOG line, an ADR sentence and a test for the boundary container that now carries scroll hints under The package and runner test results in the body are from 47b3aa1; the 47b3aa1..80ac6ae delta reads as a tidy. All checks now pass on 80ac6ae, including the iOS smoke lane that runs the rewritten route; that is not a substitute for the runs above. There are no conflicts. The next step is the live runs above and the single |
|
Live evidence for Portrait, byte-identical to main (refs stripped):
So the presentation cut over a full walk reproduces the frontier-bounded walk exactly, including the presented-depth boundary scroll hints you were watching. Landscape, keyboard up, identical on both heads: Frontier measurement (#2661 step 1). 7 warm captures per cell,
The deep-tree row is the plan recovering to Not covered: Bluesky (not installed on this machine), a physical iPhone, and the runner's walked-node count as a number — the wire carries presented counts and acquisition time, not the walk size; acquisition time is the measure of what the frontier bought. |
…eal reason the frontier went Review follow-ups on #2669. - Every CGRect becomes a SnapshotRect through SnapshotRect.init(_:). The runner's snapshotRect(from:) and the package's private SnapshotPresentationInvariant.snapshotRect(from:) were field-for-field copies; both are deleted and their nine call sites use the initializer. - ADR 0004 attributes the frontier removal to its cause: a post-walk normalization pass cannot feed a decision the walk already took, so the fold-consulting frontier had to go; the mis-cut keyboard band was the symptom. The measured cost of dropping it (form, catalog: p50 within 4 %, p95 within 10 % of main, in the branch's favour) is recorded there. - CHANGELOG line for the presentation-cut route and the capability rename.
|
Addressed on Single Live runs on this head vs a
Scroll-hint carriers under Every diff line in the runs that were not identical, explained. Two earlier passes differed and I chased both down rather than averaging them away:
Landscape, keyboard up, Step-1 frontier measurement, 7 warm captures per cell,
The frontier bought nothing measurable; the raw depth + node cap is the bound that remains. Not done, and why. Bluesky is not installed on this machine and no seeded simulator exists, so the Bluesky feed row and the |
|
Reviewed 56fcf4e, a follow-up to 80ac6ae (#2669 (comment)). The One gap is still open: the cost of Can you post All 18 checks pass on 56fcf4e, but CI does not measure this cost. There are no conflicts. The next step is the |
…ion (#2661) Move the coordinate-rotation table and SnapshotGeometrySpace into the snapshot-presentation package and add SnapshotGeometrySpace.normalized(nodes:viewport:interfaceOrientation:). Acquisition now serializes reported frames only; captureWithBackend settles every node into the app's orientation space in one ancestry-keyed pass instead of threading geometrySpace/parentIsWindow through the recursive, raw, private-AX, and query-sweep walkers. Drop the fold-consulting regular-depth frontier. It read reported (un-normalized) rects mid-walk to increment presented depth, so a turned keyboard band reported in the device's native space could be pruned before the single pass ever moved it back into the app's viewport. A regular --depth is now a presentation cut only: the walk's bounds are raw traversal depth (nil for a regular capture) plus a runaway node cap, and SnapshotPresentation applies the visibility fold and the presented-depth cut on the normalized array. Reclassify the iOS tree backend's regular-depth capability from presented-frontier to presentation-cut across the Swift declaration, the cross-runtime parity fixture, and the TypeScript registry, and remove the now-unused Swift enum case (Android's helper keeps presented-frontier). Update ADR 0004 and the ios.yml PR-lane test list.
56fcf4e to
ed75ffb
Compare
…eal reason the frontier went Review follow-ups on #2669. - Every CGRect becomes a SnapshotRect through SnapshotRect.init(_:). The runner's snapshotRect(from:) and the package's private SnapshotPresentationInvariant.snapshotRect(from:) were field-for-field copies; both are deleted and their nine call sites use the initializer. - ADR 0004 attributes the frontier removal to its cause: a post-walk normalization pass cannot feed a decision the walk already took, so the fold-consulting frontier had to go; the mis-cut keyboard band was the symptom. The measured cost of dropping it (form, catalog: p50 within 4 %, p95 within 10 % of main, in the branch's favour) is recorded there. - CHANGELOG line for the presentation-cut route and the capability rename.
|
Reviewed d1ae801, a rebase of 56fcf4e onto main after #2670. The only change to the patch is the ADR conflict resolution, and it keeps both the new keyboard-fact text and this PR's one-pass paragraph. The keyboard band from #2670 is read in the app's own space and carried beside the tree, so the new normalization pass does not change what the tap guard compares. The code is otherwise the same, so the open item from the last review (#2669 (comment)) still stands: can you post All 18 checks pass and there are no conflicts. The next step is still the |
…on the route where it engaged The earlier sentence rested on snapshot -i runs, which carry no presented depth and never engaged the frontier on main. Re-measured with --depth 2 and --depth 3 (n=15 per cell) on the recursive tier: form 135 raw nodes and scrolled catalog 279 raw nodes walked in full to present 4 or 6, acquisition p50 within ±4 % and p95 within ±7 % of main, mixed sign.
|
Addressed on You were right: Setup. Same simulator (iPhone 17 Pro, iOS 26.2), Release test app, n = 15 per cell
n = 7 per cell (the first run, kept for honesty; it is what prompted the n = 15 re-run)
Reading. At n = 7 the head looked a few ms slower at p50 and the p95 tails were wide (one 315 ms catalog outlier drives the +16.9 %); at n = 15 the sign flips per cell and every Δ is inside ±4 % p50 / ±7 % p95, so on these trees the full walk over the platform's already-materialized ADR. The "frontier bought nothing" sentence is replaced with what was measured: that the frontier engaged only under a regular |
|
Reviewed at f69fa5c. The new numbers answer the open item: they use Not blocking: All 18 checks pass, and there are no conflicts. No code blockers remain. |
thymikee
added
the
ready-for-human
|
…2684) Since #2669 the one normalization pass in captureWithBackend recomputes hittable for every acquired node, so the value flatSnapshotNode computed from the reported frame was overwritten before anything read it. The sweep now passes hittable: false like the recursive walker and private AX do, and the viewport parameter that only fed that computation goes with it. No presented output changes. Raised as a non-blocking note on #2669.
1 participant
Implements #2661: acquisition publishes reported geometry first and presentation consumes normalized geometry after a single pass. Behavior layer on top of #2664 (merged as a556e11), which owns the coordinate-rotation table and
SnapshotGeometrySpacemove.What changed
recursiveTreeSnapshotAcquisition, raw walk, private-AX, query sweep) serializes only the frame the platform reported.captureWithBackendruns a singleSnapshotGeometrySpace.normalized(nodes:viewport:interfaceOrientation:)over the flat array before any presentation reads it, keyed on ancestry — no walker carries a coordinate space (geometrySpace/parentIsWindowremoved).--depthis now a presentation cut only: the walk's bounds are raw traversal depth (nil for a regular capture) plus a runaway node cap;SnapshotPresentationapplies the visibility fold and the presented-depth cut on the normalized array (the way private-AX already did).treebackend's regular-depth capability movespresented-frontier→presentation-cutacross the Swift declaration, the cross-runtime parity fixture, and the TypeScript registry; the now-unused Swift enum case is removed (Android's helper keepspresented-frontier).normalizedlives inSnapshotCoordinateSpace.swiftcomment-free (package convention); its tests live inCoordinateSpaceTests.swift. NounplacedSurfaceHostCount, no disclosure (per the correction).Validation (head 47b3aa1)
pnpm check:affected --run: all runnable checks passed (unit suite 3011 passed, xctest-selection, fallow, format, lint, typecheck).swift test --package-path apple/snapshot-presentation: 14/14 (incl. the two-Window rotation replay, the measured landscape keyboard band, windowless-flat unplaced, and empty cases).testRegularDepthCutsPresentationNotAcquisition,testVisibilityFoldKeepsIndependentChildPastClippedParent,testRegularDepthKeepsTurnedKeyboardSubtreeAfterOneNormalizationPass(the turned-keyboard-d Nregression: the band +qare presented at the app-space rects the fix(ios): publish captured keyboard geometry in the app's orientation space #2653 fixture predicts), private-AX scroll-hint, andtestCaptureHintIsTheOnlyAcquisitionViewOfARequest— all green.-only-testing:list updated andcheck:xctest-selectionclean.Live evidence (
56fcf4e, see thread)Runner-served on iPhone 17 Pro (iOS 26.2), Release test app, vs a
maindaemon on the same simulator withopen --relaunchbefore each leg. Portrait form:-i27 = 27 (keyboard down) and 73 = 73 (up),--depth 24 = 4 / 7 = 7,--depth 36 = 6 / 10 = 10,--raw207 = 207,--json93 = 93 — all byte-identical with refs stripped; scroll-hint carrier = theCheckout formScrollView (hiddenContentBelow). Landscape keyboard up:Keyboard(75,198,724,202) and keyQ(77,203,72,45) at--depth 5/7/--json(the #2653 numbers); the only cross-leg differences anywhere were the QuickType prediction cells, an iOS per-session state, bisected across80ac6ae/56fcf4e/main to identical trees. Not covered: Bluesky (not installed), physical device.--depth Ncost (d1ae801vsmain, ADR fixed onf69fa5c)The frontier on
mainengaged only under a regular--depth N(-isends no depth), so that route was re-measured: runner-servedtree, 15 warm captures per cell,open --relaunchper leg. Catalog scrolled (279 raw nodes → 4 presented at depth 2, 6 at depth 3): p50 +2.1 % / −0.6 %, p95 +0.7 % / −1.1 %. Form (135 raw → 4 / 6): p50 −3.7 % / −2.7 %, p95 +0.9 % / −6.8 %. Mixed sign, inside noise on trees of this size; deep-tree (263 raw) recovers toprivate-axon both heads. Unmeasured: a runner-served tree in the thousands of nodes, where the node cap is the bound. Full tables in the thread.