perf: replace two quadratic scans in helpers with linear ones - #2313
perf: replace two quadratic scans in helpers with linear ones#2313zhangj23 wants to merge 1 commit into
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthrough
ChangesHelper algorithm optimization
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
49df15c to
93a18a3
Compare
Two helpers in
src/helpers.tseach run an O(n) array scan inside a loop over that same array, which makes them quadratic. Both are on paths thatcss-selectand cheerio call for every selector match, so the cost shows up on ordinary traversal work.removeSubsetsFor every ancestor of every node it called
nodes.includes(ancestor), then spliced matches out. That is O(n^2 * depth) comparisons, and each splice also reindexes the array, so the two costs compound.Membership is only ever tested against the nodes that were passed in, so they now go into a
Setonce and each ancestor check is O(1). Survivors are compacted in place.uniqueSortThe dedupe was
nodes.filter((node, index, array) => !array.includes(node, index + 1)), which is O(n^2).The obvious
new Set(nodes)is not equivalent, and this is the part I want to flag clearly: aSetkeeps the first occurrence, whileincludes(node, index + 1)keeps the last. For nodes in different documentscompareDocumentPositionreturnsDISCONNECTED, 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.I measured that difference rather than assuming it: over 20,000 randomized cross-document multisets, a first-occurrence
Setdiffers from the current behaviour on 7,079 of them. The implementation here differs on 0.Numbers
Node v24.18.0, min and median of 21 ABBA-interleaved rounds against a separately built copy of
master, on a flat sibling result set of the shapeselectAllproduces. The A/A control (two byte-identical trees) ran 0.994x to 1.008x, so anything past a few percent is real:The speedup grows with n because the quadratic term is gone, not because of a better constant factor.
Duplicate-free input gains much less, roughly 1.0x to 1.2x, because
includesshort-circuits on its first comparison there and the sort dominates. The win is for the duplicate-heavy, many-sibling collections that traversal APIs produce, which is what.parents(),.add()and.siblings()hand these helpers.Behaviour
I kept every contract the originals had, and checked each one:
uniqueSortstill skips holes in a sparse array, asfilterdid. An absent slot must not become anundefinedentry, and an earlier draft of this patch got that wrong.removeSubsetsstill keeps the first occurrence of each node, aslastIndexOf(node, index - 1)did.Setentirely in both helpers. Without that guarduniqueSort([])was about 2x slower than before, since it paid for aSetit could never use. It is now about 3x faster thanmasteron that input.Differential testing against
master:One known difference, and why I left it
Read order changes if a property getter mutates the tree or the array during the call.
removeSubsetsnow visits nodes front to back rather than back to front, anduniqueSortchecks sparse slots in the other direction. With a getter ona.parentthat setsb.parent = a:A getter on index 0 of a sparse array that materialises index 1 shows the same shape of difference. Neither is observable for ordinary input.
I tried twice to preserve the original visit order and both attempts were worse than the problem. One used a
Mapof last-seen index, which broke last-occurrence semantics, because aMapiterates in insertion order and puts a repeated node at its first position rather than its last. The other turned a wrong result into a thrownTypeError. So I stopped and am disclosing it instead.My read is that a getter which mutates the tree mid-call is not a supported pattern for these helpers, but it is a real difference and whether it matters is your call.
Checks
npm testpasses with 79 tests and exit code 0, which also runs eslint,tsc --noEmitand biome.npm run lintis clean separately.One note on motivation: the first-versus-last duplicate detail above is not hypothetical. I originally tried fixing this in cheerio with a
Setand it changed.parents()output order on multi-root selections, which is why this belongs here inuniqueSortand why the direction matters.