Skip to content

perf(find): index snapshot topology once per match ranking pass - #1971

Open
thymikee wants to merge 2 commits into
mainfrom
tier3/find-index-once
Open

perf(find): index snapshot topology once per match ranking pass#1971
thymikee wants to merge 2 commits into
mainfrom
tier3/find-index-once

Conversation

@thymikee

Copy link
Copy Markdown
Member

Closes #1690.

Summary

Find ranking rebuilt per-candidate state on any ≥2 raw matches: a fresh O(n) ancestor map per candidate, nodes.filter per same-rect step, and a viewport re-filter per candidate — plus the same resolution re-run again in classifyActionableTouchCandidates, and replay's resolveRecordedTarget double-scanning the tree on both resolved and unresolved legs.

  • New src/core/actionable-touch-topology.ts builds all three whole-tree lookups (nodes-by-index, children-by-parent, viewport-root rects) in one pass; resolveActionableTouchResolution threads it through all three previously-quadratic sites. Omitting it keeps the exact old scan path for one-off callers.
  • Ranking extracted unchanged to find-match-ranking.ts; preferOnscreenMatches is the single topology build site, with the <2 and no-root-rect early returns preserved.
  • findNearestAncestor (contracts) gains the optional prebuilt map its presentation sibling already had.
  • Replay: new resolveSelectorChainDomain returns the matched-node set the deciding pass already collected; resolveSelectorChain delegates to it and keeps its published shape; listSelectorChainMatches stays for its three other callers.
  • Ranking order preserved exactly (score inputs unchanged); pinned by literal tables plus randomized parity fixtures.

Validation

  • Red-first with operation counting: pre-fix ranking = 0 topology builds + 64 filters + 32 maps on the 32-candidate fixture; post-fix = 1 build, 0 scans. Replay legs drop their redundant filter. Mutation-checked (passing undefined for the builder turns the test red).
  • Independent adversarial review (REVIEW.md in worktree): PASS on all 12 claims — including a 12,000-tree randomized order-parity oracle against merge-base copies and a 112-combination semantic-parity fuzz of resolveSelectorChainDomain. Review finding F1 (traversal counter blind to flatMap) addressed in-commit 620a55e03; F2 (higher-traffic double scan in resolve-with-policy.ts) filed as selectors: resolveSelectorChainWithPolicy still double-scans the tree for disambiguate/fail-closed rows #1970.
  • check:affected green (602 files / 4770 tests at implementation head), fallow clean, format clean.

Internal complexity improvement only — no user-facing behavior change; docs/skills deliberately untouched.

@github-actions

github-actions Bot commented Aug 22, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 2.37 MB 2.37 MB +470 B
JS gzip 794.8 kB 795.0 kB +174 B
npm tarball 916.5 kB 916.7 kB +162 B
npm unpacked 3.18 MB 3.18 MB +470 B

npm unpacked components

Component Base Current Diff
JS / dist source 2.51 MB 2.51 MB +470 B
Apple runner source/project 564.2 kB 564.2 kB 0 B
macOS helper source 54.5 kB 54.5 kB 0 B
Android helper artifacts 0 B 0 B 0 B
Other package files 44.5 kB 44.5 kB 0 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 25.8 ms 26.2 ms +0.4 ms
CLI --help 77.6 ms 78.4 ms +0.9 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/app-inventory-contract.js +441 B +164 B
dist/src/sdk-batch-runner.js +5 B +2 B
dist/src/runtime.js 0 B +2 B

Top changed packed files

Packed file Base Current Diff
dist/src/app-inventory-contract.js 40.1 kB 40.6 kB +441 B
dist/src/find.js 7.2 kB 7.2 kB +24 B
dist/src/sdk-batch-runner.js 76.0 kB 76.1 kB +5 B

@thymikee

Copy link
Copy Markdown
Member Author

Not ready at 620a55e03:

  • Linked Index snapshot topology once when ranking find matches #1690 explicitly limits implementation/test changes to its eight-file scope, excludes selector resolution and replay guards, and says to stop rather than improvise outside scope. The packages/selectors replay-domain optimization is independent of find ranking and should be split into a separate PR. The packages/contracts helper drift also needs an explicit scope update/justification.
  • The exact-head iOS failure is an unrelated capture stall (wait_capture_stalled, readableCaptures: 0) before selector matching, but the head still needs a green rerun before merge-ready status.

The core find-topology implementation is narrow and sound; size is below thresholds (npm unpacked -10.4 kB).

@thymikee

Copy link
Copy Markdown
Member Author

The find/actionability topology optimization is sound and fully green, but this PR also bundles an independent selector/replay optimization explicitly excluded by #1690’s Scope and STOP conditions. packages/selectors/src/index.test.ts, internal/replay.ts, internal/resolve.ts, and internal/resolve.test.ts add a separate resolveSelectorChainDomain contract and replay traversal change; split those four files into their own issue/PR. The packages/contracts/src/snapshot-tree.ts optional-map seam appears necessary because the owning ancestor helper moved there, but please document that drift/update #1690’s stale path scope rather than silently broadening it. After the split, the remaining find topology slice is minimal, behavior-preserving, and merge-ready.

Move find's candidate ordering — on-screen preference, actionability
scoring, and the area/input-order tie-break — out of find-match-resolution.ts
into a focused sibling. Pure move: no score input, ranked order, ambiguity
refusal, or --first/--last behavior changes.

The extraction establishes the seam the follow-up indexing change needs,
so the ranking pass has one production entry point to build a topology in.

Refs #1690
Ranking a mutating find's candidates asked the whole tree the same three
questions once per candidate: same-rect descendants filtered every node,
the nearest hittable ancestor rebuilt a full index map, and the
overly-broad-ancestor check re-filtered every node for viewport roots.
With m matches over an n-node capture that is O(m x n) full-tree work
before find can act or refuse.

buildActionableTouchTopology reads those three collections in one pass
(nodes by index, children by parent index, normalized viewport-root
rects). preferOnscreenMatches builds exactly one per multi-match pass and
threads it through every score; resolveActionableTouchResolution takes it
as an optional argument so one-off interaction callers keep the cheap
two-argument shape. findNearestAncestor gained the same optional prebuilt
map its snapshot-presentation sibling already accepted, and
classifyActionableTouchCandidates now reuses one topology instead of
building a bare index map and re-resolving per candidate.

Score inputs, ranked order, area and input-order tie-breaks, ambiguity
refusal, and --first/--last are unchanged; only the derivation is shared.

Observed red first: with the wiring hunk removed, the new ranking
regression reports builder calls 0 (expected 1) and 64 filter + 32 map
whole-tree scans (expected 0) over 32 candidates.

The topology docstring records two seams the reviewer asked for. #1690
names src/snapshot/snapshot-processing.ts as findNearestAncestor's home;
that path is gone and packages/contracts/src/snapshot-tree.ts is the seam
that replaced it, so the issue's file list is drifted rather than a second
site to change. And viewportRootRects is not interchangeable with
snapshot-visibility's precomputedViewportRects: normalizeRect drops
negative width/height where hasValidRect keeps them, which changes which
rect wins pickLargestRect.

Refs #1690
@thymikee
thymikee force-pushed the tier3/find-index-once branch from 620a55e to aa12f07 Compare August 22, 2026 16:39
@thymikee

Copy link
Copy Markdown
Member Author

Split done at aa12f07: the selector/replay optimization moved out of this PR entirely — this branch is now the minimal find-topology slice (10 files: topology module, targeting threading, ranking extraction, contracts optional-map seam). The extracted work is on tier3/selector-replay-domain (pushed, stacked on this branch, rebases cleanly once this lands) and tracked in #1978.

Drift documentation added per your request: the contracts snapshot-tree.ts seam note records that #1690's src/snapshot/snapshot-processing.ts path no longer exists and the owning helper lives in contracts. Rebased onto current main; ranking tests green at the pushed head.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Index snapshot topology once when ranking find matches

1 participant