fix(perf): stop per-operation O(n) rescans during bulk structural edits#2801
Closed
christianhg wants to merge 2 commits into
Closed
fix(perf): stop per-operation O(n) rescans during bulk structural edits#2801christianhg wants to merge 2 commits into
christianhg wants to merge 2 commits into
Conversation
…peration `listIndexMap` (the source of `data-list-index` on rendered list items) is derived from the whole value: list-item numbering depends on block adjacency, so any structural operation could change it. The update-value subscriber rebuilt it from scratch after every operation whose path was at most two segments deep, walking all root blocks each time. A bulk edit applying N root operations therefore did O(N) full walks, i.e. O(N^2), even though nothing reads the map until the next render, and the `path.length <= 2` gate silently assumed list numbering can only live at the document root. The only reader is the text-block renderer. Structural operations now just mark the map dirty (on any path, dropping the depth assumption) and `getListIndexMap` rebuilds it on the next read. A batch of operations collapses to a single rebuild per render regardless of size, and the map is recomputed for structural changes at any depth. Net behavior unchanged: the rendered `data-list-index` values are identical; only when the rebuild happens moved from per-operation to per-read.
… 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. At the root it rebuilt a wrapper array (`value.map(...)` plus a spread) on every node. Normalizing a sibling group of n nodes therefore scanned 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 check alone, finding nothing because the keys were already unique. A sibling group's key-uniqueness only changes when its membership changes, which mints a fresh parent reference. Nested groups now cache the verified- unique verdict in a `WeakMap` keyed by parent node and re-scan only when that reference changes. The root group has no parent node (its container is the editor, whose reference never changes), so its verdict lives in `editor.rootKeysVerifiedUnique`, set after a clean scan and cleared by the op stream only when an operation can add, remove, or re-key a direct root child (`affectsRootMembership`). The scan reads the raw child array off the parent node (the field segment is already in the node's path) instead of re-resolving children through the schema, and short-circuits groups of one child, which can never collide. 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 membership condition is exact, not a depth heuristic, and over-reports rather than under-reports, so a missed duplicate is impossible.
🦋 Changeset detectedLatest commit: 23f053c 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📦 Bundle Stats —
|
| Metric | Value | vs main (525dac0) |
|---|---|---|
| Internal (raw) | 787.7 KB | +1.7 KB, +0.2% |
| Internal (gzip) | 150.4 KB | +440 B, +0.3% |
| Bundled (raw) | 1.39 MB | +1.8 KB, +0.1% |
| Bundled (gzip) | 312.3 KB | +419 B, +0.1% |
| Import time | 99ms | -2ms, -2.1% |
@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.8% |
@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, -1.7% |
@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 | 8ms | +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 | 6ms | +0ms, +3.4% |
@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 | 6ms | +0ms, +1.9% |
🗺️ . · ./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 | 41ms | +2ms, +4.3% |
🗺️ 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.
Member
Author
|
Superseded by two isolated PRs: #2802 (lazy list-index map) and #2803 (verified-unique sibling-group key scan). The duplicate-key change there is reworked to use a serialized-path set invalidated by the op stream rather than a |
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.
Inserting many blocks at once, or editing inside a large document, did work whose cost grew with the size of the whole document on every operation, so a bulk insert of N blocks spent O(N^2) in the engine before React ever rendered. Two independent per-operation rescans were responsible; this removes both. Each is its own commit.
listIndexMaprebuilt on every operationlistIndexMap(the source ofdata-list-indexon rendered list items) is derived from the whole value, because list-item numbering depends on block adjacency. The update-value subscriber rebuilt it from scratch after every operation whose path was at most two segments deep, walking all root blocks each time. A batch of N root operations therefore did N full walks, O(N^2), even though nothing reads the map until the next render, and thepath.length <= 2gate silently assumed list numbering can only live at the document root.The only reader is the text-block renderer. Structural operations now just mark the map dirty (on any path, dropping the depth assumption) and
getListIndexMaprebuilds it on the next read, so a batch collapses to a single rebuild per render and the map stays correct for structural changes at any depth.Duplicate-
_keynormalization scanned siblings per nodenormalizeNode'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 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 pre-keyed blocks spent O(n^2) here finding nothing, because the keys were already unique.A sibling group's key-uniqueness only changes when its membership changes, which mints a fresh parent reference. Nested groups now cache the verified-unique verdict in a
WeakMapkeyed by parent node and re-scan only when that reference changes. The root group has no parent node (its container is the editor, whose reference never changes), so its verdict lives ineditor.rootKeysVerifiedUnique, set after a clean scan and cleared by the op stream only when an operation can add, remove, or re-key a direct root child (affectsRootMembership). The scan itself reads the raw child array straight off the parent node (the field segment is already in the node's path) instead of re-resolving children through the schema, and short-circuits groups of one child, which can never collide. The membership condition is exact, not a depth heuristic, and over-reports rather than under-reports, so a missed duplicate is impossible.Blast radius
Net behavior is unchanged: rendered
data-list-indexvalues are identical, and duplicate keys are still detected and the same later occurrence re-keyed in the same order. Only when the work happens changed (per-operation to per-read for the list map; once-per-group instead of once-per-node for the key scan). The full unit suite (801) and the chromium (1645) and webkit (1624) browser suites pass;tests/focus.test.tsxfails only in firefox and fails identically onmain(it passes in isolation; a pre-existing focus-stealing flake), so it is untouched by this change.Chromium, headless, medians of 3 runs,
tests/performance.test.tsx:The remaining time is dominated by React mounting the 1000 components, which is inherent and untouched.