fix(perf): scan each sibling group for duplicate keys once instead of per node#2803
Draft
christianhg wants to merge 1 commit into
Draft
fix(perf): scan each sibling group for duplicate keys once instead of per node#2803christianhg wants to merge 1 commit into
christianhg wants to merge 1 commit into
Conversation
… per node normalizeNode's duplicate-`_key` fix resolved one node at a time: for each dirty node it fetched the node's siblings and scanned them for a key collision, and at the root it rebuilt a wrapper array (`value.map(...)` plus a spread) per node. Normalizing a sibling group of n nodes was O(n^2), and at the root allocated O(n) throwaway arrays per node, so a bulk insert of n pre-keyed blocks spent O(n^2) here finding nothing. A sibling group's key-uniqueness only changes when its own direct membership changes. `editor.verifiedUniqueChildGroups` records the serialized path of every group (a node's keyed child array, or the root value array as `''`) verified to hold no duplicate keys, and normalizeNode skips the scan for a listed group. The op stream removes a group only when an operation inserts, removes, re-keys, or replaces one of its direct children, so an edit deep inside one group never forces a re-scan of its ancestors, and an operation that introduces a subtree also removes that subtree's groups so a reused key can't inherit a stale verdict. The id is read from the operation path directly (keyed on every hot path); a numeric segment, or an unusual whole-value `set`, clears the set, which only costs extra scans. Groups of one child can't collide, so they are neither scanned nor cached; only genuinely multi-child groups pay a serialized id. The scan reads the raw child array off the parent node rather than re-resolving children through the schema. Works identically at every depth, root and container alike, with no weak references. Net behavior unchanged: duplicate keys are detected and the same later occurrence is re-keyed, in the same order; only the redundant rescans are removed.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 870511a The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
📦 Bundle Stats —
|
| Metric | Value | vs main (525dac0) |
|---|---|---|
| Internal (raw) | 788.9 KB | +2.9 KB, +0.4% |
| Internal (gzip) | 150.6 KB | +582 B, +0.4% |
| Bundled (raw) | 1.39 MB | +2.9 KB, +0.2% |
| Bundled (gzip) | 312.4 KB | +583 B, +0.2% |
| Import time | 87ms | +0ms, +0.2% |
@portabletext/editor/behaviors
| Metric | Value | vs main (525dac0) |
|---|---|---|
| Internal (raw) | 467 B | - |
| Internal (gzip) | 207 B | - |
| Bundled (raw) | 424 B | - |
| Bundled (gzip) | 171 B | - |
| Import time | 2ms | -0ms, -0.0% |
@portabletext/editor/plugins
| Metric | Value | vs main (525dac0) |
|---|---|---|
| Internal (raw) | 2.7 KB | - |
| Internal (gzip) | 894 B | - |
| Bundled (raw) | 2.5 KB | - |
| Bundled (gzip) | 827 B | - |
| Import time | 7ms | +0ms, +0.8% |
@portabletext/editor/selectors
| Metric | Value | vs main (525dac0) |
|---|---|---|
| Internal (raw) | 78.5 KB | - |
| Internal (gzip) | 14.4 KB | - |
| Bundled (raw) | 74.0 KB | - |
| Bundled (gzip) | 13.3 KB | - |
| Import time | 7ms | +0ms, +0.7% |
@portabletext/editor/traversal
| Metric | Value | vs main (525dac0) |
|---|---|---|
| Internal (raw) | 24.6 KB | - |
| Internal (gzip) | 4.9 KB | - |
| Bundled (raw) | 24.5 KB | - |
| Bundled (gzip) | 4.8 KB | - |
| Import time | 5ms | +0ms, +2.5% |
@portabletext/editor/utils
| Metric | Value | vs main (525dac0) |
|---|---|---|
| Internal (raw) | 28.8 KB | - |
| Internal (gzip) | 6.0 KB | - |
| Bundled (raw) | 26.7 KB | - |
| Bundled (gzip) | 5.7 KB | - |
| Import time | 5ms | +0ms, +0.5% |
🗺️ . · ./behaviors · ./plugins · ./selectors · ./traversal · ./utils · Artifacts
Details
- Import time regressions over 10% are flagged with
⚠️ - Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.
📦 Bundle Stats — @portabletext/markdown
Compared against main (525dac02)
| Metric | Value | vs main (525dac0) |
|---|---|---|
| Internal (raw) | 53.0 KB | - |
| Internal (gzip) | 9.6 KB | - |
| Bundled (raw) | 347.6 KB | - |
| Bundled (gzip) | 96.0 KB | - |
| Import time | 34ms | -0ms, -0.9% |
🗺️ View treemap · Artifacts
Details
- Import time regressions over 10% are flagged with
⚠️ - Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Duplicate-
_keynormalization innormalizeNoderesolved one node at a time: for every dirty node it fetched that node's siblings and scanned them for a key collision, and at the root it rebuilt a wrapper array (value.map(...)plus a spread) on every node. Normalizing a sibling group of n nodes was therefore O(n^2), and at the root allocated O(n) throwaway arrays per node, so a bulk insert of n pre-keyed blocks spent O(n^2) in this one check, finding nothing because the keys were already unique.The fix turns the per-node rescan into one scan per group. A sibling group's key-uniqueness only changes when its own direct membership changes, so
editor.verifiedUniqueChildGroupsrecords the serialized path of every group (a node's keyed child array, or the root value array as'') verified to hold no duplicate keys, andnormalizeNodeskips the scan for a listed group. The op stream removes a group only when an operation inserts, removes, re-keys, or replaces one of its direct children. An operation that introduces a subtree also removes that subtree's groups, so a key reused after a deletion can't inherit a stale verdict. Groups of one child can't collide, so they are neither scanned nor cached. The scan reads the raw child array off the parent node instead of re-resolving children through the schema.The group id is read from the operation path directly, which is keyed on every hot path, so invalidation needs no walk of the value from the root; a numeric path segment or an unusual whole-value
setclears the set, which only costs extra scans.The mechanism is uniform at every depth, root and container alike, and uses no weak references. That generality is not just tidiness: it fixes a real cliff for large containers. Identifying a group by its parent node's reference would re-verify the group whenever any descendant changed (structural sharing mints a fresh parent on every deep edit), so editing one cell of a thousand-row table would re-scan all thousand rows. Keyed by serialized path and invalidated only on the group's own membership change, an edit deep inside one group never re-verifies its ancestors.
Net behavior is unchanged: duplicate keys are still detected and the same later occurrence re-keyed, in the same order; only the redundant rescans are removed. The full unit suite (801) and chromium (1645) and webkit (1624) browser suites pass.
This is one of two extracted performance PRs. The other (
editor-perf-lazy-list-index) defers the list-index map rebuild; the two target different costs and are additive. They touch the same engine fields (subscribeUpdateValue, the engine type, engine setup), so whichever merges second needs a trivial rebase.Chromium, headless, medians of 3 runs,
tests/performance.test.tsx, this change alone vsmain:The duplicate scan was roughly this share of the insert path, so removing it caps out here; together with the list-index PR the two land in the 12-14% range.