[combobox][select] Make the multiple selection anchor lookup linear - #5613
Conversation
`findSelectionIndex` resolved the multi-selection anchor by rescanning the whole selected-value array for every rendered item, making the lookup O(items x selections). Before mui#5573 it was a single O(items) pass. `findIndex` short-circuits on the first hit, so the common case still costs one comparison; the blow-up needs no rendered item to be selected, which happens when a large list filters every selected value out of the rendered window. At 5,000 items x 2,500 filtered-out selections that is 12.5M comparisons and ~64ms in one synchronous call. Index the selected values in a `Set` when the comparer is the default `Object.is`, so each item is a constant-time probe. A custom `isItemEqualToValue` may call values equal that do not hash alike, so it keeps the linear scan. Four behaviours are preserved. `Set` membership is SameValueZero and unifies `+0` with `-0`, so a hit on a zero falls back to an exact `Object.is` scan, which is what keeps the anchor agreeing with the `isSelected` store selectors. `undefined` and sparse holes are filtered out of the index so they never match. The index is built with `forEach` rather than iterated, so it snapshots the length and skips holes exactly as the `some()` scan it replaces did, and an array carrying its own `Symbol.iterator` resolves the same anchor as before. And the nullish path through `compareItemEquality` is untouched, since the fast path only runs when the comparer already is `Object.is`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
commit: |
Bundle size
PerformanceTotal duration: 1,071.11 ms +47.84 ms(+4.7%) | Renders: 76 (+0) | Paint: 1,723.09 ms +66.41 ms(+4.0%)
13 tests within noise — details Metric alarms
Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
In the PR, I figured this was not a realistic or worthwhile scenario given the mount cost of 5,000 items dominates Edit: nevermind, it actually affects virtualized lists as well |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Shorten the comments around the indexed anchor lookup and drop the references to how the lookup used to be written, which a reader arriving at this file has no way to check. Comments and one test comment only; no behavior change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
atomiks
left a comment
There was a problem hiding this comment.
Independent measurement:
| Scenario | Base A1 / A2 | PR head |
|---|---|---|
| 5,000 items × 2,500 filtered-out selections | 21.88 / 21.97 ms | 0.115 ms |
| 500 items × 50 filtered-out selections | 0.0508 / 0.0539 ms | 0.00469 ms |
| 5,000 items × 2,500 selections, first-item hit | <0.001 ms | 0.0420 ms |
Brings in 13 commits up to 2e1bddd, notably mui#5613 ([combobox][select] Make the multiple selection anchor lookup linear), which overlaps this branch's Select and Combobox selection logic. Conflicts resolved: - combobox/root/AriaCombobox.tsx — kept mui#5613's toggled-index anchor lookup, wrapped in this branch's `updateActiveIndexState` helper; both the new `clearedBySelection` branch and this branch's disabled-aware always-highlight branch preserved. - combobox/item/ComboboxItem.tsx — combined mui#5613's `resolveSelectedIndex` import with this branch's virtualization imports. - internals/resolveValueLabel.test.ts — combined this branch's added exports with upstream's vitest-import refactor. Merge follow-ups: - select/root/SelectRoot.tsx — the import auto-merge dropped `findItemIndex` (still used by the virtualized prune) in favor of mui#5613's `findSelectionIndex`; both are now imported. - Added vitest global imports across the branch's test files and hoisted one env-conditional expect out of an if/else — upstream tightened `vitest/prefer-importing-vitest-globals` and `vitest/no-conditional-expect`. Gates green: typecheck, eslint, prettier; jsdom Select/Virtualizer/Combobox/ Autocomplete/resolveValueLabel; Chromium Select (only the pre-existing focus flake), Combobox, Virtualizer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Brings in 13 commits up to 2e1bddd, notably mui#5613 ([combobox][select] Make the multiple selection anchor lookup linear), which overlaps this branch's Select and Combobox selection logic. Conflicts resolved: - combobox/root/AriaCombobox.tsx — kept mui#5613's toggled-index anchor lookup, wrapped in this branch's `updateActiveIndexState` helper; both the new `clearedBySelection` branch and this branch's disabled-aware always-highlight branch preserved. - combobox/item/ComboboxItem.tsx — combined mui#5613's `resolveSelectedIndex` import with this branch's virtualization imports. - internals/resolveValueLabel.test.ts — combined this branch's added exports with upstream's vitest-import refactor. - select/root/SelectRoot.tsx — the import auto-merge dropped `findItemIndex` (still used by the virtualized prune) in favor of mui#5613's `findSelectionIndex`; both are imported. - Vitest global imports added across this branch's test files and one env-conditional expect hoisted out of an if/else, for upstream's tightened `vitest/prefer-importing-vitest-globals` and `vitest/no-conditional-expect`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The regression
#5573 changed
findSelectionIndexto anchor multi-selection to the first selected itemin rendered order. Getting that ordering right meant asking, for every rendered item,
"is this one selected?" — and the selected values are a plain array, so each question
became a full scan:
That is O(items × selections). In 1.7.0 the same function did
findItemIndex(itemValues, lastValue, comparer)— one O(items) pass. Both Select andCombobox route through it (
SelectRoot.tsx:244,AriaCombobox.tsx:462,1027,1115).findIndexshort-circuits on the first hit, so the common case — some rendered item isselected, usually near the top — still costs a single comparison. The blow-up needs no
rendered item to be selected, which is what happens when a large list has filtered every
selected value out of the rendered window.
Comparison counts at 5,000 × 2,500: 1.7.0 = 5,000, current = 12,500,000, this PR = 0.
Be clear about the size of this: outside large lists it is sub-millisecond, and the
common case was never slow. This is a complexity regression worth closing, not an
emergency.
The fix
Index the selected values in a
Setand probe it in constant time — but only when thecomparer is the default
Object.is. A customisItemEqualToValuecan call values equalthat do not hash alike, so it cannot be indexed and keeps the existing nested scan. The
helper is module-private; no signature changed and nothing new is exported.
The last two rows above are the honest cost: the
Setis built eagerly whilefindIndexshort-circuits, so the fast path adds up to ~0.04 ms in the common case — and only at
selection counts where the bad case costs ~64 ms. Below ~500 selections it is not
measurable. A hybrid (probe the first K items, build the index only on a miss) would
remove even that, at the price of a magic constant and two more branches; that did not
seem worth it.
Correctness constraints preserved
Setmembership is SameValueZero;Object.isis not. They differ only on+0vs
-0. ASethit on a value that is=== 0falls back to an exactObject.isscan. This is not pedantry: the
isSelectedstore selectors (select/store.ts,combobox/store.ts) compare throughObject.is, so without the re-check the anchorcould land on an item that renders without
data-selected.undefinednever matches.selectedValueIncludesrejectsselectedValue === undefinedandfindItemIndexrejectsitemValue === undefined.Sparse arrays are real here — items
deletetheir registry slot on unmount — so holesand explicit
undefinedare both filtered out while building the index.compareItemEqualityroutes any nullish operand toObject.is, never to thecomparer. Untouched by construction: the fast path only runs when the comparer
already is
Object.is, and the slow path is unchanged.A fourth came out of review: the index is built with
forEach, not by iterating. Thatmatches how the
some()scan it replaces consumed the values — same length snapshot,same hole skipping, no
Symbol.iterator— so an array carrying its own iterator resolvesthe same anchor as before instead of a different one (or throwing, if that property is
nulled).
Tests
itemEquality.test.tsgoes from 9 to 16 tests. The load-bearing one counts how manytimes the selected values are read, via a
Proxy, and asserts it stays linear. It waschecked against two deliberately quadratic implementations — the pre-#5573 nested scan
and a variant that rebuilds the index inside the item loop — and fails on both at 20,000
reads versus a 300 budget. (An earlier version counted
Object.iscalls instead; reviewcaught that the fast path never calls
Object.is, so that assertion was satisfied by 0and passed the quadratic variant too.)
The rest pin behavioural equivalence with the pre-fix scan:
±0in both directions,NaN,null,undefined, sparse holes on both sides, a custom comparer through themultiple-mode path, and the
Symbol.iteratorbuild. All of them pass against the oldimplementation as well — only the cost changed, which is the point.
Two independent reviewers ran differential fuzzing against the pre-fix source: ~250,000
and ~45,000 randomized cases respectively, over a pool including
±0,NaN,±Infinity,null,undefined, BigInt, symbols, functions, boxed primitives, objects and holes.Zero divergences.
Known remaining cost, not addressed here
resolveSelectedIndexstill does O(selections) work per item in a layout effect(
ComboboxItem.tsx:121,SelectItem.tsx:83), which is also a #5573 regression. It isleft alone deliberately, and one reviewer disagreed with that call — so here is the
evidence, measured on a rendered 200-item × 100-selection multiple Select:
isSelectediterationsresolveSelectedIndexiterationsThe resolver runs one pass, at item mount only — its effect deps contain no selected
value. The
isSelectedstore selector recomputes for every mounted item on every storenotification. #5573 did not touch either store file, so that path was already
O(items × selections); #5573 took mount from 3 passes to 4.
Fixing the resolver alone would need a membership index shared across items — a
store-level derived value or a
WeakMapkeyed on the value array — and would buy ~25% atmount and 0% elsewhere while leaving the dominant pass in place. It also carries a hazard
this PR's per-call
Setcannot have:isSelected's result identity drivesuseStore'sgetSnapshotchange detection, so a stale shared index would make items silently missre-renders. That wants to be one follow-up covering
isSelected,resolveSelectedIndexand
findSelectionIndextogether, with tests for the staleness and zero cases.Two smaller things also left as-is: the
±0fallback scan is unbounded, so the fast pathis "linear except for zeros" (it needs thousands of item slots holding
±0with theopposite sign selected, which no real list produces); and nothing pins that production
wiring actually reaches the fast path, which is selected by reference identity to a
default parameter — wrapping
isItemEqualToValueanywhere would silently revert theoptimization with a green suite.
itemCollection.ts:14already has the same unguardedfragility, so a shared guard would cover both.
🤖 Generated with Claude Code