fix: Sync NODE_TO_INDEX and NODE_TO_PARENT on apply instead on render

[delijah]

Description
This PR will keep NODE_TO_INDEX and NODE_TO_PARENT in sync right after applying operations, instead of on render. This will make the DOMEditor.findPath function more reliable.

Issue
Fixes: #5938

Context
Please have a look at the issue for more context.

Checks

• The new code matches the existing patterns and styles.
• The tests pass with yarn test.
• The linter passes with yarn lint. (Fix errors with yarn fix.)
• The relevant examples still work. (Run examples with yarn start.)
• You've added a changeset if changing functionality. (Add one with yarn changeset add.)

[changeset-bot]

⚠️ No Changeset found

Latest commit: 098fa53

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[12joan]

The main problem with updating NODE_TO_INDEX and NODE_TO_PARENT on every apply is performance. Due to the way weak map keys are implemented in browsers, both reading from and writing to weak maps is very slow, and doing so on every operation is very likely to have a measurable impact on performance.

On of the main focuses of my performance PR was reducing the number of weak map operations, which is why getChunkTreeForNode is used when chunking is enabled to avoid setting the weak maps unnecessarily.

You could probably do the same thing at the apply level by figuring out which nodes' parents and indices are changed by each operation and updating the minimum number of weak map entries to account for this.

[delijah]

Ah. I assumed inside matches we already have only the nodes that were affected by the operation?

[12joan]

Ah. I assumed inside matches we already have only the nodes that were affected by the operation?

matches doesn't exactly equal the set of nodes whose paths will change. For example, it a node is inserted or removed, all future sibling nodes' NODE_TO_INDEX must be updated. These sibling nodes are not currently included in matches.

Ideally, we would handle each operation type individually to determine whether NODE_TO_INDEX and/or NODE_TO_PARENT needs updating. This would also require very careful testing, since it can be unintuitive in some cases. For example, an insert_text operation, although it doesn't cause any node's path to change, still requires updating the weak maps for the affected leaf node and all of its ancestors, since these nodes' objects will be immutably replaced with fresh objects.

Regarding that last point, it may be a good idea if we used NODE_KEY_TO_INDEX and NODE_KEY_TO_PARENT_KEY instead, where the "key" is the one from NODE_TO_KEY. Keys are stable for the lifetime of a node, so the weak maps for computing paths would need updating much less frequently. Alternatively, we could simply store the entire path in a single weak map NODE_TO_PATH.

[delijah]

Regarding that last point, it may be a good idea if we used NODE_KEY_TO_INDEX and NODE_KEY_TO_PARENT_KEY instead, where the "key" is the one from NODE_TO_KEY. Keys are stable for the lifetime of a node, so the weak maps for computing paths would need updating much less frequently. Alternatively, we could simply store the entire path in a single weak map NODE_TO_PATH.

In the same time you wrote your answer, i've create a new commit. Were you talking about something like that?

Inpired by https://webreflection.medium.com/a-basic-weakmap-performance-hint-e33c14908dff

[12joan]

Broadly, yes, although using Key['id'] as a key would lead to a memory leak. It needs to be a weak map where the key is the Key object itself.

[delijah]

Nice. Ok, added the changes. But yes, the complex part will be finding out which paths are affected and also testing this logic. Will not have time for now to take care about this 😢

[delijah]

For example, an insert_text operation, although it doesn't cause any node's path to change, still requires updating the weak maps for the affected leaf node and all of its ancestors, since these nodes' objects will be immutably replaced with fresh objects.

Not sure if i understand you correctly. What needs to be updated on insert_text? You're saying the node keys change for the affected leaf node and all of its ancestors? Therefore we have to set the paths for the new node keys?

[12joan]

For example, an insert_text operation, although it doesn't cause any node's path to change, still requires updating the weak maps for the affected leaf node and all of its ancestors, since these nodes' objects will be immutably replaced with fresh objects.

Not sure if i understand you correctly. What needs to be updated on insert_text? You're saying the node keys change for the affected leaf node and all of its ancestors? Therefore we have to set the paths for the new node keys?

That part of my comment was assuming that the weak maps were keyed on the node object, not the node key object. Since node objects are completely replaced when their or their descendant's value changes, any weak maps keyed on the node object need to be updated. That isn't necessary when keying on the key object.