From fbba7ed50362d0e184b2943f73fac1e4615c005a Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Wed, 9 Sep 2026 20:29:00 -0400 Subject: [PATCH] perf(studio): resolve a selector's occurrence index once per layer walk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The preview flattens several composition files into one DOM, so a layer's identity is its selector plus its occurrence index WITHIN its own source file. `getSourceScopedSelectorIndex` derived that per element: a whole -document `querySelectorAll(selector)`, `resolveSourceFile` on every match, then `indexOf`. Every element sharing a class paid for all of them, so a walk over n such elements did n whole-document queries and n^2 source-file resolutions — the shape a composition of repeated cards or tiles has by construction. Build the occurrence index ONCE per selector instead and share it across one walk. `withSelectorIndexPass(doc, run)` opens that scope; outside it the helper behaves exactly as before, per call. The pass lives in `collectDomEditLayerItems`, which owns the loop, rather than at a call site — its four callers (off-canvas indicators, the layers panel, the marquee hit-test and the agent look tool) all walked the same way and all paid the same cost. Behaviour is unchanged. The occurrence indices are identical, including the misses: an element outside the requested source file, or one not matching the selector, still yields undefined, as do `#`-prefixed and `[data-composition-id=` selectors and an invalid selector. Measured on a 1689-element preview over 80 single-frame seeks, per rebuild: class-selector document queries 171.6 -> 10.8, and the walk's self-timed cost 15.25ms -> 5.37ms. Elements walked per rebuild is unchanged at 973.7, so the two arms did the same work. Tests assert complexity invariance rather than a threshold: the query count must be IDENTICAL at n and 4n elements sharing a selector, which a fixture -sized threshold would not catch. Both fail on the previous algorithm. --- .../editor/domEditingLayers.test.ts | 48 ++++++ .../src/components/editor/domEditingLayers.ts | 12 +- ...CanvasIndicatorGeometry.complexity.test.ts | 117 ++++++++++++++ .../utils/sourceScopedSelectorIndex.test.ts | 148 ++++++++++++++++++ .../src/utils/sourceScopedSelectorIndex.ts | 70 ++++++++- 5 files changed, 387 insertions(+), 8 deletions(-) create mode 100644 packages/studio/src/components/editor/offCanvasIndicatorGeometry.complexity.test.ts create mode 100644 packages/studio/src/utils/sourceScopedSelectorIndex.test.ts diff --git a/packages/studio/src/components/editor/domEditingLayers.test.ts b/packages/studio/src/components/editor/domEditingLayers.test.ts index e5bb830704..6e80a0fffb 100644 --- a/packages/studio/src/components/editor/domEditingLayers.test.ts +++ b/packages/studio/src/components/editor/domEditingLayers.test.ts @@ -257,3 +257,51 @@ describe("collectDomEditLayerItems item budget", () => { expect(collectDomEditLayerItems(documentWith(200), opts, 80)).toHaveLength(80); }); }); + +describe("collectDomEditLayerItems selector-index cost", () => { + // Attached, unlike the fixture above: a detached subtree is invisible to + // document.querySelectorAll, so the occurrence lookup would find nothing. + function attachedRootWithSharedClass(count: number): HTMLElement { + const root = document.createElement("div"); + root.setAttribute("data-composition-id", "index.html"); + for (let i = 0; i < count; i++) { + const child = document.createElement("div"); + child.className = "box"; + root.append(child); + } + document.body.append(root); + return root; + } + + /** Class-selector document queries made by one walk over `count` sibling cards. */ + function classSelectorQueries(count: number): number { + const root = attachedRootWithSharedClass(count); + const doc = root.ownerDocument; + const real = doc.querySelectorAll.bind(doc); + let calls = 0; + Object.defineProperty(doc, "querySelectorAll", { + configurable: true, + value: (selector: string) => { + if (selector.startsWith(".")) calls += 1; + return real(selector); + }, + }); + try { + expect(collectDomEditLayerItems(root, opts)).toHaveLength(count); + return calls; + } finally { + delete (doc as Partial).querySelectorAll; + root.remove(); + } + } + + // The occurrence index is resolved here, for every item, so an unshared index + // costs one whole-document query per element — quadratic once a composition + // repeats a card or tile class. Owning the pass here rather than at each call + // site is what keeps the layers panel, the marquee and the agent's look tool + // linear too; invariance across a 4x fixture fails for any per-element term. + it("resolves a shared selector once per walk, not once per element", () => { + expect(classSelectorQueries(48)).toBe(classSelectorQueries(12)); + expect(classSelectorQueries(12)).toBe(1); + }); +}); diff --git a/packages/studio/src/components/editor/domEditingLayers.ts b/packages/studio/src/components/editor/domEditingLayers.ts index 77805259b4..32c5e394f2 100644 --- a/packages/studio/src/components/editor/domEditingLayers.ts +++ b/packages/studio/src/components/editor/domEditingLayers.ts @@ -32,6 +32,7 @@ import { getSelectionCandidate, } from "./domEditingElement"; import { isCompositionRootLayer } from "./domEditingRootLayer"; +import { withSelectorIndexPass } from "../../utils/sourceScopedSelectorIndex"; export function isEditableTextLeaf(el: HTMLElement): boolean { return isTextBearingTag(el.tagName.toLowerCase()) && el.children.length === 0; @@ -475,8 +476,15 @@ export function collectDomEditLayerItems( } }; - // Drilled into a group → show only its members; otherwise the whole tree. - for (const el of groupScopedLayerRoots(root, options.activeGroupElement ?? null)) visit(el, 0); + // Every item resolves its selector's occurrence index, and unshared that is a + // whole-document query per element — quadratic once a composition repeats a + // card or tile class. The walk is one synchronous read of a document it does + // not mutate, so one index per selector serves the whole of it. The pass lives + // here rather than in each caller because this function owns the loop. + withSelectorIndexPass(root.ownerDocument, () => { + // Drilled into a group → show only its members; otherwise the whole tree. + for (const el of groupScopedLayerRoots(root, options.activeGroupElement ?? null)) visit(el, 0); + }); return items; } diff --git a/packages/studio/src/components/editor/offCanvasIndicatorGeometry.complexity.test.ts b/packages/studio/src/components/editor/offCanvasIndicatorGeometry.complexity.test.ts new file mode 100644 index 0000000000..d3e5c4da56 --- /dev/null +++ b/packages/studio/src/components/editor/offCanvasIndicatorGeometry.complexity.test.ts @@ -0,0 +1,117 @@ +// @vitest-environment happy-dom + +import type React from "react"; +import { afterEach, describe, expect, it } from "vitest"; +import type { OffCanvasRect } from "./OffCanvasIndicators"; +import { recomputeOffCanvasIndicators } from "./offCanvasIndicatorGeometry"; + +const realGetBoundingClientRect = Element.prototype.getBoundingClientRect; +afterEach(() => { + Element.prototype.getBoundingClientRect = realGetBoundingClientRect; +}); + +interface Rebuild { + /** querySelectorAll calls on the preview document, per selector. */ + queriesBySelector: Map; + /** The indicator keys, which carry each element's selector occurrence index. */ + keys: string[]; +} + +/** + * One real rebuild over a preview whose `cardCount` cards all share `.box`, + * counting the preview document's queries. Everything below the entry point is + * production code: the layer walk, the patch targets, and the selector + * occurrence indices that end up in each indicator's key. + */ +function rebuildWithSharedSelector(cardCount: number): Rebuild { + const iframe = document.createElement("iframe"); + document.body.append(iframe); + const doc = iframe.contentDocument; + if (!doc) throw new Error("Expected iframe content document"); + + const cards = Array.from( + { length: cardCount }, + (_unused, i) => `
card ${i}
`, + ).join(""); + doc.body.innerHTML = `
${cards}
`; + + const overlay = document.createElement("div"); + document.body.append(overlay); + + // Cards sit left of the composition, so every one of them is off-canvas and + // reaches the indicator list; everything else measures empty and does not. + Element.prototype.getBoundingClientRect = function (): DOMRect { + if (this === iframe || this === overlay) return new DOMRect(0, 0, 800, 450); + if (this instanceof doc.defaultView!.Element && this.classList.contains("box")) { + return new DOMRect(-500, 40, 100, 40); + } + return new DOMRect(0, 0, 0, 0); + }; + + const queriesBySelector = new Map(); + const realQuerySelectorAll = doc.querySelectorAll.bind(doc); + Object.defineProperty(doc, "querySelectorAll", { + configurable: true, + value: (selector: string) => { + queriesBySelector.set(selector, (queriesBySelector.get(selector) ?? 0) + 1); + return realQuerySelectorAll(selector); + }, + }); + + const sigRef = { current: "" } as React.MutableRefObject; + const elementsRef = { current: new Map() } as React.MutableRefObject< + Map + >; + let rects: OffCanvasRect[] = []; + + recomputeOffCanvasIndicators( + iframe, + overlay, + doc, + { left: 0, top: 0, width: 800, height: 450 }, + "index.html", + sigRef, + elementsRef, + (next) => { + rects = next; + }, + ); + + iframe.remove(); + overlay.remove(); + return { queriesBySelector, keys: rects.map((rect) => rect.key) }; +} + +/** The queries that resolve a class selector's occurrence index — the work this + * guards. Excluded: the per-element `[data-composition-id]` ancestor lookup and + * the stylesheet scan, which are linear per element and a separate seam. */ +function classSelectorQueries(rebuild: Rebuild): Array<[string, number]> { + return [...rebuild.queriesBySelector].filter(([selector]) => selector.startsWith(".")).sort(); +} + +describe("recomputeOffCanvasIndicators selector-index cost", () => { + // The defect this guards: the occurrence index used to be resolved with its + // own whole-document querySelectorAll PER element, so n cards sharing a class + // cost n queries and n² source-file resolutions. A threshold would pass on a + // small fixture while a real composition runs hundreds of cards. Invariance + // across a 4x fixture cannot — it fails for any per-element term at all. + it("resolves shared selectors with the same number of queries at n and at 4n", () => { + const small = rebuildWithSharedSelector(12); + const large = rebuildWithSharedSelector(48); + + expect(classSelectorQueries(large)).toEqual(classSelectorQueries(small)); + expect(classSelectorQueries(small)).toEqual([ + [".box", 1], + [".label", 1], + ]); + }); + + it("still numbers every shared-selector element in document order", () => { + for (const cardCount of [12, 48]) { + const { keys } = rebuildWithSharedSelector(cardCount); + expect(keys).toEqual( + Array.from({ length: cardCount }, (_unused, i) => `index.html:.box:${i}`), + ); + } + }); +}); diff --git a/packages/studio/src/utils/sourceScopedSelectorIndex.test.ts b/packages/studio/src/utils/sourceScopedSelectorIndex.test.ts new file mode 100644 index 0000000000..20e1fe72d8 --- /dev/null +++ b/packages/studio/src/utils/sourceScopedSelectorIndex.test.ts @@ -0,0 +1,148 @@ +// @vitest-environment happy-dom + +import { describe, expect, it } from "vitest"; +import { getSourceScopedSelectorIndex, withSelectorIndexPass } from "./sourceScopedSelectorIndex"; + +// The per-element algorithm this helper used before it grew a per-pass index, +// transcribed verbatim. It is the independent source the memoized results are +// checked against — including the misses, which are `indexOf` returning -1. +function referenceIndex( + doc: Document, + el: Element, + selector: string | undefined, + sourceFile: string | undefined, + resolveSourceFile: (candidate: Element) => string | undefined, +): number | undefined { + if (!selector || selector.startsWith("#") || selector.startsWith("[data-composition-id=")) { + return undefined; + } + try { + const scope = sourceFile ?? "index.html"; + const matches = Array.from(doc.querySelectorAll(selector)).filter( + (candidate) => (resolveSourceFile(candidate) ?? "index.html") === scope, + ); + const matchIndex = matches.indexOf(el); + return matchIndex >= 0 ? matchIndex : undefined; + } catch { + return undefined; + } +} + +const bySourceFile = (candidate: Element): string | undefined => + candidate.closest("[data-composition-file]")?.getAttribute("data-composition-file") ?? undefined; + +// Two source files sharing one class, plus an unscoped element, is the case +// source-file scoping exists for: raw document order would number these 0..4. +function mixedSourceDocument(): Document { + const doc = document.implementation.createHTMLDocument("mixed"); + doc.body.innerHTML = ` +
+
+

a0

+

a1

+
+
+

b0

+

b1

+
+

root0

+
+ `; + return doc; +} + +describe("getSourceScopedSelectorIndex", () => { + it("numbers occurrences within each source file, not across the flattened document", () => { + const doc = mixedSourceDocument(); + const read = (id: string, sourceFile: string | undefined) => + getSourceScopedSelectorIndex(doc, doc.getElementById(id)!, ".box", sourceFile, bySourceFile); + + expect(read("a0", "a.html")).toBe(0); + expect(read("a1", "a.html")).toBe(1); + expect(read("b0", "b.html")).toBe(0); + expect(read("b1", "b.html")).toBe(1); + expect(read("root0", undefined)).toBe(0); + // Asking for an element under the wrong scope is a miss, not a neighbour's index. + expect(read("b0", "a.html")).toBeUndefined(); + }); + + it("matches the per-element reference for every element, inside a pass and outside it", () => { + const doc = mixedSourceDocument(); + const boxes = Array.from(doc.querySelectorAll(".box")); + const scopes = [undefined, "index.html", "a.html", "b.html", "missing.html"]; + const cases = boxes.flatMap((el) => scopes.map((scope) => ({ el, scope }))); + + const expected = cases.map(({ el, scope }) => + referenceIndex(doc, el, ".box", scope, bySourceFile), + ); + + const outsidePass = cases.map(({ el, scope }) => + getSourceScopedSelectorIndex(doc, el, ".box", scope, bySourceFile), + ); + const insidePass = withSelectorIndexPass(doc, () => + cases.map(({ el, scope }) => + getSourceScopedSelectorIndex(doc, el, ".box", scope, bySourceFile), + ), + ); + + expect(outsidePass).toEqual(expected); + expect(insidePass).toEqual(expected); + }); + + it("returns undefined for the selectors that carry their own identity", () => { + const doc = mixedSourceDocument(); + const el = doc.getElementById("a0")!; + for (const selector of [undefined, "#a0", '[data-composition-id="root"]']) { + expect( + getSourceScopedSelectorIndex(doc, el, selector, "a.html", bySourceFile), + ).toBeUndefined(); + expect( + withSelectorIndexPass(doc, () => + getSourceScopedSelectorIndex(doc, el, selector, "a.html", bySourceFile), + ), + ).toBeUndefined(); + } + }); + + it("returns undefined for an invalid selector rather than throwing", () => { + const doc = mixedSourceDocument(); + const el = doc.getElementById("a0")!; + expect(getSourceScopedSelectorIndex(doc, el, ".((", "a.html", bySourceFile)).toBeUndefined(); + expect( + withSelectorIndexPass(doc, () => + getSourceScopedSelectorIndex(doc, el, ".((", "a.html", bySourceFile), + ), + ).toBeUndefined(); + }); + + it("does not reuse one document's index for another, and restores the outer pass", () => { + const inner = mixedSourceDocument(); + const outer = mixedSourceDocument(); + // A nested pass over a different document must not answer from, or poison, + // the outer document's index. + const nested = withSelectorIndexPass(outer, () => + withSelectorIndexPass(inner, () => + getSourceScopedSelectorIndex( + inner, + inner.getElementById("b1")!, + ".box", + "b.html", + bySourceFile, + ), + ), + ); + expect(nested).toBe(1); + + const afterNested = withSelectorIndexPass(outer, () => { + withSelectorIndexPass(inner, () => undefined); + return getSourceScopedSelectorIndex( + outer, + outer.getElementById("a1")!, + ".box", + "a.html", + bySourceFile, + ); + }); + expect(afterNested).toBe(1); + }); +}); diff --git a/packages/studio/src/utils/sourceScopedSelectorIndex.ts b/packages/studio/src/utils/sourceScopedSelectorIndex.ts index 6627e1a5e1..15b04dbe7d 100644 --- a/packages/studio/src/utils/sourceScopedSelectorIndex.ts +++ b/packages/studio/src/utils/sourceScopedSelectorIndex.ts @@ -5,6 +5,60 @@ * `querySelectorAll` index is not a stable source-file identity. Callers supply * their existing source resolver; this helper alone owns occurrence scoping. */ + +/** Every element matching one selector, mapped to the source file it belongs to + * and its occurrence index within that file. One document query fills it. */ +type SelectorOccurrences = Map; + +interface SelectorIndexPass { + doc: Document; + bySelector: Map; +} + +let activePass: SelectorIndexPass | null = null; + +/** + * Share one occurrence index per selector across every + * `getSourceScopedSelectorIndex` call made synchronously inside `run`. + * + * Unshared, each call runs its own whole-document `querySelectorAll(selector)` + * and resolves the source file of every match. A rebuild that walks n elements + * sharing a selector therefore does n whole-document queries and n² resolver + * calls — a composition of repeated cards or tiles is exactly that shape. One + * index per selector makes the same pass linear. + * + * Correctness rests on the pass being ONE synchronous walk of a document that + * does not change under it: nothing invalidates the index, so `run` must not + * mutate `doc`, and every call inside it must resolve source files the same way + * (the resolver may be a fresh closure per call, as long as it answers the + * same). Outside a pass the index is per-call, exactly as before. + */ +export function withSelectorIndexPass(doc: Document, run: () => T): T { + const previous = activePass; + activePass = { doc, bySelector: new Map() }; + try { + return run(); + } finally { + activePass = previous; + } +} + +function buildOccurrences( + doc: Document, + selector: string, + resolveSourceFile: (candidate: Element) => string | undefined, +): SelectorOccurrences { + const occurrences: SelectorOccurrences = new Map(); + const counts = new Map(); + for (const candidate of doc.querySelectorAll(selector)) { + const scope = resolveSourceFile(candidate) ?? "index.html"; + const index = counts.get(scope) ?? 0; + counts.set(scope, index + 1); + occurrences.set(candidate, { scope, index }); + } + return occurrences; +} + export function getSourceScopedSelectorIndex( doc: Document, el: Element, @@ -17,12 +71,16 @@ export function getSourceScopedSelectorIndex( } try { - const scope = sourceFile ?? "index.html"; - const matches = Array.from(doc.querySelectorAll(selector)).filter( - (candidate) => (resolveSourceFile(candidate) ?? "index.html") === scope, - ); - const matchIndex = matches.indexOf(el); - return matchIndex >= 0 ? matchIndex : undefined; + const pass = activePass?.doc === doc ? activePass : null; + let occurrences = pass?.bySelector.get(selector); + if (!occurrences) { + occurrences = buildOccurrences(doc, selector, resolveSourceFile); + pass?.bySelector.set(selector, occurrences); + } + // An element outside the requested scope is not in that scope's occurrence + // run at all, which is the `indexOf` miss the filtered form returned before. + const hit = occurrences.get(el); + return hit && hit.scope === (sourceFile ?? "index.html") ? hit.index : undefined; } catch { return undefined; }