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.delete and transform.insertFragment performance optimize #5137

Merged
merged 3 commits into from
Oct 19, 2022
Merged

transform.delete and transform.insertFragment performance optimize #5137

merged 3 commits into from
Oct 19, 2022

Conversation

mainhanu
Copy link
Contributor

@mainhanu mainhanu commented Sep 26, 2022

Description
delete and insert large fragment can be very time consuming,the time complexity is O(n2)

Example

import { createEditor, Editor, Transforms } from "slate";
import type { Descendant } from "slate";

const createP = (i: string = '') => ({
  type: "p",
  children: [
    { text: `${i} This is editable ` },
    { text: 'rich', bold: true },
    { text: ' text, ' },
    { text: 'much', italic: true },
    { text: ' better than a ' },
    { text: '<textarea>', code: true },
    { text: '!' },
  ],
})

// one thousand paragraphs
const largeFragment: Descendant[] = new Array(2000).fill(1).map((_, i) => createP(i + ''));

export const testDelete = () => {
  const editor = createEditor();
  editor.children = largeFragment;

  const sel = Editor.range(editor, []);
  Transforms.select(editor, sel);

  console.time("delete");
  Transforms.delete(editor);
  console.timeEnd("delete");
};

environment

  • MacBook Pro (13-inch, 2019) 2.4 GHz 4 Intel Core i5
  • Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/105.0.0.0 Safari/537.36

result: 2000 pargagraphs

seq time(millisecond)
1 12210.78
2 12882.65
3 13567.65
4 13315.84
average 12994.23

but with 200 pargagraphs

seq time(millisecond)
1 191
2 179
3 177
4 180
average 181

Context
The reason is related to the number of path refs in the apply process.

for transform.delete, The following code causes each apply to process the path transform of the first i-1 paragraphs.

for (const pathRef of pathRefs) {
    const path = pathRef.unref()!
    Transforms.removeNodes(editor, { at: path, voids })
}

we can remove nodes from last to first to avoid use path ref.

const middlePaths = pathRefs
  .reverse()
  .map(r => r.unref())
  .filter((r): r is Path => !!r)

for (const p of middlePaths) {
  Transforms.removeNodes(editor, { at: p, voids })
}

for transform.insertFragment, the O(n2) has something todo with the dirtyPath logic, each insertNode will add new dirtyPath to DIRTY_PATHS and subsquent apply will transform all previous dirtyPath。I hadn't figure out how to opitimize it。according to the logic, move large fragment may also has the same problem。

I just add a tiny optimization to Path.transform, array destruction is faster than immer's produce, and it also make remove 2000 paragraphs 3x faster

after optimize

delete 2000 paragraphs

seq time(millisecond)
1 896
2 843
3 853
4 850
average 860

delete 200 paragraphs

seq time(millisecond)
1 49
2 46
3 47
4 44
average 46

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 Sep 26, 2022

🦋 Changeset detected

Latest commit: 317fa6b

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

@bryanph
Copy link
Contributor

bryanph commented Sep 28, 2022

Ultimately we need operations like insert_nodes and remove_nodes that operate on a range to make this more performant I think (#2288)

@mainhanu
Copy link
Contributor Author

mainhanu commented Sep 29, 2022

Ultimately we need operations like insert_nodes and remove_nodes that operate on a range to make this more performant I think (#2288)

I think current Transform.insertFragment and Transform.delete includes scenario like insert_nodes and removeNodes。we should optimize these methods,like how to optimze the cumulative dirtypath transform logic。

create new Operations will affect OT transform logic,which will case more problems to consider

@dylans
Copy link
Collaborator

dylans commented Oct 12, 2022

@bryanph any further thoughts after #5137 (comment) ?

@bryanph
Copy link
Contributor

bryanph commented Oct 17, 2022

@dylans Not really, overall this seems like a valid optimization to me

edit: I do have one question actually, do we know why this is so slow with immer? It's just operating on a flat array, seems like that should be comparable in performance to destructuring and applying changes to that?

@dylans
Copy link
Collaborator

dylans commented Oct 18, 2022

edit: I do have one question actually, do we know why this is so slow with immer? It's just operating on a flat array, seems like that should be comparable in performance to destructuring and applying changes to that?

They used to not recommend immer on arrays with thousands of items, but that doesn't seem to be part of their performance tips any more: https://immerjs.github.io/immer/performance#performance-tips

@bryanph
Copy link
Contributor

bryanph commented Oct 19, 2022

@dylans based on this section it might just be that in these benchmarksproduce() is called a whole lot of times which creates the overhead 🤷

@dylans dylans merged commit a2184d8 into ianstormtaylor:main Oct 19, 2022
This was referenced Oct 19, 2022
z2devil pushed a commit to z2devil/slate that referenced this pull request Dec 6, 2024
…anstormtaylor#5137)

* feat: transform.delete and transform.insertFragment performance optimize

* feat: add changeset

* feat: optimize code

Co-authored-by: mainhanu <chijun89@gmail.com>
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.

4 participants