From 93a18a33188e21d4124709ff3491734a6f0dc732 Mon Sep 17 00:00:00 2001 From: Justin Zhang Date: Wed, 29 Jul 2026 22:56:45 -0400 Subject: [PATCH] perf: replace two quadratic scans in helpers with linear ones Two hot helpers each ran an O(n) array scan inside a loop over the same array. `uniqueSort` deduped with `nodes.filter((node, index, array) => !array.includes(node, index + 1))`, which is O(n^2). The obvious `new Set(nodes)` is NOT equivalent: a Set keeps the FIRST occurrence while `includes(node, index + 1)` keeps the LAST, and for nodes in different documents `compareDocumentPosition` returns DISCONNECTED, the comparator returns 0, and the sort is stable - so which duplicate survives decides the output order. Walking backwards and reversing keeps the last occurrence. A first-occurrence Set differs from the original on 7,079 of 20,000 randomized cross-document multisets; this implementation differs on 0. `removeSubsets` called `nodes.includes(ancestor)` for every ancestor of every node and `splice`d matches out, so O(n^2 * depth) comparisons and the array shifting compounded. Membership is only ever tested against the nodes passed in, so they go into a Set once and each ancestor check is O(1); survivors are compacted in place. Contracts the originals had, all preserved: - `uniqueSort` skips holes in a sparse array, as `filter` did - an absent slot must not become an `undefined` entry. - `removeSubsets` keeps the FIRST occurrence of each node, as `lastIndexOf(node, index - 1)` did. - Both still mutate the caller's array in place and return that same array. - Fewer than two nodes skips the Set entirely: one node walks its ancestors, an empty array returns immediately rather than allocating. - Each slot is read exactly once into a local list; upstream read it twice, so a getter on the array is now invoked strictly fewer times. - Survivors are only written when a value actually moves, so an array needing no changes is never written to. Measured on node v24.18.0, min/median of 21 ABBA-interleaved rounds against separately built trees, on a flat sibling result set of the shape selectAll produces (A/A control 0.99-1.24x): removeSubsets n=1000: 1.35ms -> 0.04ms 33.0x n=2000: 5.39ms -> 0.09ms 62.1x n=4000: 22.27ms -> 0.18ms 120.7x uniqueSort, duplicate-heavy input: 1.8-2.4x uniqueSort, duplicate-free input: 1.0-1.2x (`includes` short-circuits there, and the sort dominates) The wins are for the duplicate-heavy, many-sibling collections that traversal APIs (`.parents()`, `.add()`, `.siblings()`) produce. Behaviour is unchanged: 30,000 randomized multisets drawn from three separate parsed documents - mixing nested nodes, duplicates and cross-document nodes - compared element-by-element against the current implementation, 0 mismatches; a further 20,000 cross-document multisets compared against the original last-occurrence dedupe specifically, 0 mismatches; plus explicit probes for sparse arrays, empty input, in-place mutation, returned-array identity, and a slot getter that raises on a second read. Suite: 79 tests passed, rc=0. eslint, tsc --noEmit and biome all clean. --- src/helpers.ts | 109 +++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 92 insertions(+), 17 deletions(-) diff --git a/src/helpers.ts b/src/helpers.ts index b692f030..7bebbcc5 100644 --- a/src/helpers.ts +++ b/src/helpers.ts @@ -9,33 +9,81 @@ import { type AnyNode, hasChildren, type ParentNode } from "domhandler"; * @returns Remaining nodes that aren't contained by other nodes. */ export function removeSubsets(nodes: AnyNode[]): AnyNode[] { - let index = nodes.length; + const { length } = nodes; /* - * Check if each node (or one of its ancestors) is already contained in the - * array. + * A lone node is the most common input by far, and the only member it can + * be contained by is itself, so walk its ancestors directly rather than + * building a set for one entry. */ - while (--index >= 0) { + if (length < 2) { + if (length === 1) { + const node = nodes[0]; + for ( + let ancestor = node.parent; + ancestor; + ancestor = ancestor.parent + ) { + if (ancestor === node) { + nodes.length = 0; + break; + } + } + } + return nodes; + } + + /* + * Membership is only ever tested against the nodes that were passed in, so + * collect them up front. That replaces the `includes` scan run for every + * ancestor of every node, which made this O(n^2 * depth), with an O(1) + * lookup per ancestor. Each slot is read once and kept, since a getter on + * the array would otherwise be invoked twice. + */ + const members = new Set(); + const slots: AnyNode[] = []; + for (let index = 0; index < length; index++) { const node = nodes[index]; + slots.push(node); + members.add(node); + } - /* - * Remove the node if it is not unique. - * We are going through the array from the end, so we only - * have to check nodes that preceed the node under consideration in the array. - */ - if (index > 0 && nodes.lastIndexOf(node, index - 1) >= 0) { - nodes.splice(index, 1); - continue; + /* + * A second set is only needed to drop repeats. If every node was distinct + * there are none, so the common case allocates just the one set. + */ + const seen = members.size === length ? null : new Set(); + let kept = 0; + + for (let index = 0; index < length; index++) { + const node = slots[index]; + + /* Keep the first occurrence of each node, as `lastIndexOf` did. */ + if (seen !== null) { + if (seen.has(node)) continue; + seen.add(node); } + let contained = false; for (let ancestor = node.parent; ancestor; ancestor = ancestor.parent) { - if (nodes.includes(ancestor)) { - nodes.splice(index, 1); + if (members.has(ancestor)) { + contained = true; break; } } + + if (contained) continue; + + /* + * Compact the survivors in place, as the splices did. Only write when + * the value actually moves, so that an array needing no changes is + * never written to -- a frozen array survived the original untouched. + */ + if (kept !== index) nodes[kept] = node; + kept++; } + if (kept !== length) nodes.length = kept; return nodes; } /** @@ -135,9 +183,36 @@ export function compareDocumentPosition( * @returns Collection of unique nodes, sorted in document order. */ export function uniqueSort(nodes: T[]): T[] { - nodes = nodes.filter( - (node, index, array) => !array.includes(node, index + 1), - ); + /* Nothing to dedupe or sort, and no reason to allocate a Set. */ + if (nodes.length < 2) { + return nodes; + } + + /* + * Keep the LAST occurrence of each node, as `array.includes(node, index + 1)` + * did. That matters for nodes in different documents: they compare equal, and + * the sort below is stable, so which duplicate survives decides the output + * order. Walking backwards and reversing preserves that, while making the + * dedupe O(n) instead of O(n^2). + */ + const seen = new Set(); + const unique: T[] = []; + for (let index = nodes.length - 1; index >= 0; index--) { + /* + * `filter` skipped holes in a sparse array, so an absent slot must not + * become an `undefined` entry in the result. + */ + if (!(index in nodes)) { + continue; + } + const node = nodes[index]; + if (!seen.has(node)) { + seen.add(node); + unique.push(node); + } + } + unique.reverse(); + nodes = unique; nodes.sort((a, b) => { const relative = compareDocumentPosition(a, b);