Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transform.insertNodes & Transform.insertFragment performance optimize #5543

Merged
merged 6 commits into from
Feb 7, 2024

Conversation

mainhanu
Copy link
Contributor

@mainhanu mainhanu commented Oct 31, 2023

This PR is a subsequent optimization of the previous PR.

Why does insertNodes/insertFragment have performance issues?

Each editor instance has a corresponding dirtyPaths, used to judge which nodeEntry to execute normalizeNode.

  1. Op will generate dirtyPath. For example, when insert a node, Path.levels(path) and node descendants paths will be added to dirtyPaths
  2. When not batched, normalizeNode will be executed on every apply
  3. When batching (withoutNormalizing), normalizeNode will be delayed, and the existing dirtyPaths need to be path transformed with subsequent ops on each apply.

Since slate is a collaborative editor based on OT, an insertNodes/insertFragment command will be converted into many Ops. for example, insertNodes with n nodes will eventually be converted into n insertNode Ops

Assume there is no dirtyPath before insertNodes, n text nodes are inserted, and the depth of inserted path is d

  1. When inserting the 1st text, d dirtyPaths are generated
  2. When inserting the second text, transform the previous d dirtyPaths, execute Path.levels(p) once, and the total number of dirtyPaths is d+1. (according to Path.transform logic)
  3. When inserting the third text, transform the previous d + 1 dirtyPaths, execute Path.levels(p) once, and the total number of dirtyPaths is d+2
  4. ...
  5. When inserting the nth text, transform the previous d + n - 2 dirtyPaths, execute Path.levels(p) once, and the total number of dirtyPaths is d + n - 1

so finally (n - 1) * d + d * (n - 2) * (n - 1) / 2 times of path transform and n - 1 times of Path.levels were executed. The complexity is O(n2)

If you insert n block nodes with complex children or into a higher depth, the time will also increase multiply.

how to optimize

For insertNodes. It is not necessary to perform transform on the dirtyPath generated by the preceding insertNode op. because:

  1. insertNode is inserted sequentially under the same parent. The subsequent insertNode Op has no effect on the dirtyPath generated by the previous insertNodeOp. (Path.compare result is 1)
  2. In addition, insertNode is executed under the same parent, and there is no need to execute Path.levels every time

So we can batch update the dirtyPaths of the n node insert. Proceed as follows:

  1. Transform the existing dirtyPaths in sequence according to insertNode Ops.
  2. Unified calculation of new dirtyPaths generated by insertNodes

and In order to implement this , we need to allow dirty paths batch update outside of apply。

benchmark

test code

export function benchTest(n) {
  const editor = createEditor();
  editor.children = [];

  const nodes = new Array(n)
    .fill(0)
    .map((i) => ({
      type: "paragraph",
      children: [{ text: "Hello World " + i }],
    }));

  console.time();
  editor.insertNodes(nodes);
  console.timeEnd();
}

test environment

  • MacBook Pro (14-inch, 2020) Apple M1
  • Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/118.0.0.0 Safari/537.36

result

n before(ms) after(ms)
100 10 2
500 65 15
1000 260 45
5000 6600 920
10000 28000 3500

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
Copy link

changeset-bot bot commented Oct 31, 2023

🦋 Changeset detected

Latest commit: d01c912

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
slate Minor

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

Copy link
Collaborator

@dylans dylans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks promising, will review asap

@mainhanu
Copy link
Contributor Author

mainhanu commented Nov 7, 2023

added batchDirty to insertNodes & insertFragment options for unit test, to make sure with or without batchDirty, the normalization paths are the same

@mainhanu
Copy link
Contributor Author

mainhanu commented Dec 1, 2023

Looks promising, will review asap

hey, any progress about this?

@dylans
Copy link
Collaborator

dylans commented Feb 7, 2024

Looks promising, will review asap

hey, any progress about this?

Apologies, life was a bit hectic recently. Will look at this shortly.

@dylans dylans merged commit 3aaf3b5 into ianstormtaylor:main Feb 7, 2024
6 of 11 checks passed
@github-actions github-actions bot mentioned this pull request Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants