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

[Bug]: Performance issue with large documents migrating from vue 2 to vue 3 #5031

Closed
2 tasks done
Rirax opened this issue Apr 4, 2024 · 30 comments
Closed
2 tasks done
Labels
Category: Open Source The issue or pull reuqest is related to the open source packages of Tiptap. Type: Bug The issue or pullrequest is related to a bug

Comments

@Rirax
Copy link
Contributor

Rirax commented Apr 4, 2024

Which packages did you experience the bug in?

@tiptap/vue-3

What Tiptap version are you using?

2.2.4

What’s the bug you are facing?

Hello everyone,

In my company, we're fervent Tiptap users, and Tiptap pro subscribers. Today we're experiencing a blocking performance issue with tiptap/vue-3.

Since migrating from tiptap/vue-2 to tiptap/vue-3, we've noticed a slowdown on documents with a large number of nodes that need to be rendered in Vue.

A look at the thread on tiptap-vue3 implementation](#1166 (comment)) mentions the breaking changes between vue-2 and vue-3, and the use of Teleport to add elements to the DOM, which may explain this drop in performance.

To demonstrate the bug, you need content with at least 1500 nodes.

The loading time increases quadratically with the number of nodes.

The repository below reproduces the bug 👇

What browser are you using?

Chrome

Code example

https://codesandbox.io/p/devbox/tiptap-vue-3-vuenodeviewrenderer-mg4dp8?file=%2Fsrc%2FApp.vue%3A16%2C1

What did you expect to happen?

We are a knowledge management software company, and our applications are particularly dependent on tiptap for the user experience.

We are in the process of migrating our stack from Vue 2 to Vue 3 and as a result are looking to adopt tiptap/vue-3.
The current performance issue is blocking our migration to Vue3.

Anything to add? (optional)

Let us know if we can be of any help to reproduce this bug or contribute a fix to it.

Did you update your dependencies?

  • Yes, I’ve updated my dependencies to use the latest version of all packages.

Are you sponsoring us?

  • Yes, I’m a sponsor. 💖
@Rirax Rirax added Category: Open Source The issue or pull reuqest is related to the open source packages of Tiptap. Type: Bug The issue or pullrequest is related to a bug labels Apr 4, 2024
@github-project-automation github-project-automation bot moved this to Triage open in Tiptap Apr 4, 2024
@bdbch
Copy link
Member

bdbch commented Apr 6, 2024

Thanks for reporting! I tried to get a quick grasp via performance debugging in Chrome Dev Tools but seems like it even crashes my chrome instance on that.

I just saw that the NodeViewRenderer takes a lot of time for each component - I don't have a Vue 2 setup now that I could easily test. Do you still have your code around that you could put into a sandbox real quick so I can do my tests on that to compare where and what increases the rendering time that much?

@bdbch
Copy link
Member

bdbch commented Apr 6, 2024

Quick test (which I don't think is reliable) shows that each VueRenderer's constructor takes around 9 to 13ms which adds up real quick on 1500 elements.

@Rirax
Copy link
Contributor Author

Rirax commented Apr 8, 2024

Hi @bdbch ,

Thank you for your reply !

For comparison, here is the same content with tiptap vue-2.

I've inspected the vue-3 package and I think the performance problem comes partly from the render method of EditorContent.ts.

`render() {
const vueRenderers: any[] = []

if (this.editor) {
  this.editor.vueRenderers.forEach(vueRenderer => {
    const node = h(
      Teleport,
      {
        to: vueRenderer.teleportElement,
        key: vueRenderer.id,
      },
      h(
        vueRenderer.component as DefineComponent,
        {
          ref: vueRenderer.id,
          ...vueRenderer.props,
        },
      ),
    )

    vueRenderers.push(node)
  })
}

return h(
  'div',
  {
    ref: (el: any) => { this.rootEl = el },
  },
  ...vueRenderers,
)

}`

It appears that with each successive update to add a new node, all previously rendered nodes are rendered each time.

Here a PR Draft

I hope this will help you solve this bug.

Thank you very much for your help!

@bdbch bdbch linked a pull request Apr 8, 2024 that will close this issue
4 tasks
@SevrainChea
Copy link

We are also using tiptap (vue2) in my company and we are starting the process of migrating to vue3. Thank you @Rirax for highlighting this performance issue !
I'm posting to follow this thread, but this could potentially be a hard blocker for us because we currently have clients with several thousand lines documents....

@bdbch Any clues regarding the timeline of this fix ? 🙏

Thanks in advance for your work

@bdbch
Copy link
Member

bdbch commented Apr 10, 2024

No timeline yet – @Rirax send in a PR which I'll check asap when I find time to see if it improves rendering performance and doesn't break anything – after that we'll see to release it in a new minor if everything is working fine.

But can't give a time estimate rn.

@Bv-Lucas
Copy link

Hello everyone,
I'm facing the same issue at my company when moving from tiptap/vue2 to tiptap/vue3.
Thanks for the investigation @Rirax and for the PR.
I hope this will get fixed soon so we can complete our migration, is there still no timeline defined @bdbch ?
Thanks everyone and have a good weekend

@fushugaku
Copy link

Hello, tried to build tiptap with pr from @Rirax and to add it to our project that uses a lot of VueNodeViewRenderer components, unfortunately didn't work for us, the tab just crashes, though it worked almost flawless with vue2 :/
(we use tiptap for big articles, about 2-6mb of json content)
The project itself has almost no getters/watchers, even when I removed them all - the browser tab just freezes.
Anyway, thanks everyone for their hard work, I hope this issue will be resolved sooner or later, since it's a major blocker for transitioning to vue 3.

@timurerrant
Copy link

Hello, tried to build tiptap with pr from @Rirax and to add it to our project that uses a lot of VueNodeViewRenderer components, unfortunately didn't work for us, the tab just crashes, though it worked almost flawless with vue2 :/ (we use tiptap for big articles, about 2-6mb of json content) The project itself has almost no getters/watchers, even when I removed them all - the browser tab just freezes. Anyway, thanks everyone for their hard work, I hope this issue will be resolved sooner or later, since it's a major blocker for transitioning to vue 3.

Yep, same here. Were using it on vue2 and got those issues with vue3. Tried to solve it by myself but no luck for now. Honestly still hope for an update. Thanks for the work again!

@AndreewMelnik
Copy link

Hi! Thanks @Rirax for writing about this problem.
We also use tiptap in our company, and we started having the same problem since move from vue2 to vue3.
Performance on large documents began to suffer greatly, some simply stopped opening because of this. I hope for a fix soon and thanks everyone for your work!

@galibov
Copy link

galibov commented May 3, 2024

Hello, I also tried integrating Tiptap with the PR from @Rirax into our Vue 3 project and faced similar issues with the browser tab crashing, despite it working well with Vue 2

@SevrainChea
Copy link

Hi @bdbch, it's been a month since this thread has been opened and it seems to be a critical issue for several tiptap users for transitioning from vue2 to vue3

Any news on the timeline ? Thanks in advance for your work

@Veseron
Copy link

Veseron commented May 6, 2024

I see many problems similar to mine, but I decided to share anyway. A couple of days ago we completed the migration of the project to Vue 3 and started getting tab crashes when there was a large amount of content

Maybe there is some intermediate solution for us? Tried to make a build from pr above, but it did not fix the issue.

@koalest
Copy link

koalest commented May 6, 2024

Hi! I've also stumbled upon this issue, unfortunately, vue2 performance was much better, it's a major blocker for us also. Hope there is some kind of solution, thank you for all your hard work!

@Rirax
Copy link
Contributor Author

Rirax commented Jun 3, 2024

Hello everyone,

Thank you all for your feedback on this issue, which helps move things forward.

@bdbch thanks for the review. I took a closer look at the error test for the Drawing example and it turns out that the component needs to rerender at each line to be able to take update props into account.

I therefore revised my proposal, removing the use of Teleport and the need to rerender the app for each node.
I took my inspiration from the vue-2 package to enable each component to be rendered as a unit and added to tiptap.
#5206

According to my tests, I've noticed a clear improvement in performance for content with a lot of nodes to be rendered by Vue.
Capture d’écran 2024-06-03 à 22 03 27

I'm interested in your feedback to see if you're seeing the same thing as me or if there's still room for improvement.

@fushugaku
Copy link

@Rirax, thank you, we'll try your solution tomorrow in our project, I've also thought that the problem was with vue-teleport, but unfortunately had no time to investigate, thank you for you hard work.

@fushugaku
Copy link

@Rirax, we made a few pre-tests with json content of ~6mb, works perfectly for us, big thanks!

@Rirax
Copy link
Contributor Author

Rirax commented Jun 4, 2024

Hi @fushugaku,
Thanks for your feedback.
I'm glad to hear it fixes your performance issues too !

@fushugaku
Copy link

fushugaku commented Jun 5, 2024

Hi @Rirax, we've found another issue - when deleting vue nodes via deleteNode editor doesn't delete the node, we are investigating, but no luck yet.
// upd
found the problem, it was on our side, we were deleting component in onBeforeUnmount, but it's already handled now

@nperez0111
Copy link

Hi everyone, I failed to mention that we have released @Rirax's fix to our beta v2.5.0-beta.4

Please try it out & let us know if you run into any issues.

@Rirax
Copy link
Contributor Author

Rirax commented Jun 18, 2024

Hi @nperez0111, thank you for the release !

@peter-lobner
Copy link

Hi @nperez0111,

as the possible fix in the meantime seams to be reverted from the release branch and therefore not included in the latest releases, when can we expect the fix to be officially released?

@nperez0111
Copy link

Yea I opened another branch with the vue fixes. I need to test them a bit more carefully as some vue users ran into issues with it.

Probably on the order of a week you can expect an official release back into 2.5 minor

@Rirax
Copy link
Contributor Author

Rirax commented Jul 19, 2024

Hi @nperez0111 ,
I've fixed the last issue with this MR.

This code has been in production on our side for 3 weeks and we've had no more problems.

I'm available if needed to provide a solution if other problems arise and bring these changes in the official package quickly.

@nperez0111
Copy link

Hi @nperez0111 , I've fixed the last issue with this MR.

This code has been in production on our side for 3 weeks and we've had no more problems.

I'm available if needed to provide a solution if other problems arise and bring these changes in the official package quickly.

Thank you again for your contribution @Rirax, I got word about other issues with vue so I wanted to spend the time to validate it separately. I will let you know if I need any help with them. It also could be some bugs that I accidentally introduced on the 2.5 release that may have just been noise. Things have slowed down since the release so I'll let it soak over the weekend & go from there

@nperez0111
Copy link

I've re-released this with 2.5.5

Please let me know if you run into anymore issues

@github-project-automation github-project-automation bot moved this from Triage open to Done in Tiptap Jul 22, 2024
@GTarkin
Copy link

GTarkin commented Jul 23, 2024

Hello @nperez0111,

i tried fix using tiptap 2.5.5 and have run into an issue with it, but i am rather unsure if the problem is the fix or its how I use tiptap/prosemirror.

In my application I have two editor instances realizing a split editor where the user can see and edit the same document in two different places. I use the 'onTransaction' event hook to replicate state between the two editors to keep both editors in sync. Part of this replication is also replicating the selection state which breaks now that the fix is applied. It seems the editor to which the selection should be replicated to still has the old state when the replication happens, despite having received the new state of the other editor within the same transaction. In this old state the selection position is not a valid document position which causes prosemirror to throw an exception. This also seems to affect some already existing Plugins that I use to manipulate prosemirror-transactions with, which now seem to see a different selection within the applied transactions than they did before. Partly I was able to fix the replication issue by using the 'onUpdate' hook of the editor instead of 'onTransaction', but the issue with the broken plugin remains.

Do you maybe have an idea why this might be is the case after the update?

@nperez0111
Copy link

Hi @GTarkin, so you are saying that the selection state is behind in your replicated editor? I'm unsure how that would be affected by this change, but it sounds like it would have been caused by this: #5252

Honestly, this use-case seems a bit odd to me. I wouldn't have thought that you could even propagate selection updates like that much less keep them in sync. I would need to see code for how that is integrated

@GTarkin
Copy link

GTarkin commented Jul 23, 2024

Hi @nperez0111

the ticket you linked looks promising, though i don't know yet if it will work when using the newly introduced beforeTransaction-hook. Currently I am occupied with another thing. A colleague will try it out tomorrow and will inform you.

Attached is the actual replication code that syncs the editors. Maybe this helps anyway in pinpointing it down. If not then there will be a possbility for me or my colleague to create a minimal example in the next days, hopefully on Stackblitz

export type KnownStep = ReplaceStep
  | ReplaceAroundStep
  | AddMarkStep
  | RemoveMarkStep;

export function isKnownStep(step: Step): step is KnownStep {
  return step instanceof ReplaceStep
    || step instanceof ReplaceAroundStep
    || step instanceof AddMarkStep
    || step instanceof RemoveMarkStep;
}

export function filterForUnknownSteps(transaction: Transaction): Step[] {
  return transaction.steps.filter((step: Step) => !isKnownStep(step));
}

export function syncEditors(sourceEditor: EditorView, targetEditor: EditorView, transaction: Transaction): boolean {

  const debug = false;

  const log = (msg: string, ...additionalOutput: any) => {
    if (debug) {
      console.log(msg, additionalOutput);
    }
  }

  if(transaction.getMeta(ProsemirrorTransactionMeta.REPLICATED_TRANSACTION)){
    log('transaction already replicated from other editor');
    return true;
  }

  const replicatedTransaction = targetEditor.state.tr;
  log('start replication');

  const nonReplicatedSteps = filterForUnknownSteps(transaction);

  if (nonReplicatedSteps.length > 0) {
    log('There are steps that cant be replicated.', nonReplicatedSteps);
    return false;
  }

  transaction.steps
    .filter(isKnownStep)
    .forEach((step: KnownStep) => {
      if (step instanceof ReplaceStep) {
        const replacestep = step as ReplaceStep;
        const adaptedSlice = Slice.fromJSON(targetEditor.state.schema, replacestep.slice.toJSON()!);
        const newReplaceStep = new ReplaceStep(replacestep.from, replacestep.to, adaptedSlice);
        log('replacing', replacestep, newReplaceStep);
        const {doc, failed} = replicatedTransaction.maybeStep(newReplaceStep);
        if (failed) {
          const s = debugPrintProseMirrorStructure(sourceEditor.state.doc).reduce(
            (accumulator, currentValue) => accumulator + currentValue +
              '\n', '');
          const t = debugPrintProseMirrorStructure(targetEditor.state.doc).reduce(
            (accumulator, currentValue) => accumulator + currentValue +
              '\n', '');
          log(s);
          log(t);
          log(`1 - could not replicate step: ${step.toJSON()}`);
          return false;
        }
      } else if (step instanceof ReplaceAroundStep) {
        const replaceAroundStep = step as ReplaceAroundStep;
        const adaptedSlice = Slice.fromJSON(targetEditor.state.schema, replaceAroundStep.slice.toJSON()!);
        const newReplaceAroundStep = new ReplaceAroundStep(replaceAroundStep.from, replaceAroundStep.to,
                                                           replaceAroundStep.gapFrom, replaceAroundStep.gapTo,
                                                           adaptedSlice,
                                                           replaceAroundStep.insert,
                                                           (replaceAroundStep as any).structure);
        const {doc, failed} = replicatedTransaction.maybeStep(newReplaceAroundStep);
        if (failed) {
          log(`2 - could not replicate step: ${step.toJSON()}`);
          return false;
        }
      } else if (step instanceof AddMarkStep) {
        const addMarkStep = step as AddMarkStep;
        const replicatedAddMarkStep = new AddMarkStep(addMarkStep.from, addMarkStep.to, addMarkStep.mark);
        const {doc, failed} = replicatedTransaction.maybeStep(replicatedAddMarkStep);
        if (failed) {
          log(`3 - could not replicate step: ${step.toJSON()}`);
          return false;
        }
      } else if (step instanceof RemoveMarkStep) {
        const removeMarkStep = step as RemoveMarkStep;
        const replicatedRemoveMarkStep = new AddMarkStep(removeMarkStep.from, removeMarkStep.to, removeMarkStep.mark);
        const {doc, failed} = replicatedTransaction.maybeStep(replicatedRemoveMarkStep);
        if (failed) {
          log(`4 - could not replicate step: ${step.toJSON()}`);
          return false;
        }
      } else {
        const exhaustiveCheck: never = step;
        throw new Error(`Unhandled step: ${exhaustiveCheck}`);
      }
    });

 // if I comment this snipped out the logic works again, even though the replicated characters arrive in 'wrong' order at the target. 
// Words appear right to left instead of left to right
 // <-- snip -->
  const currentSelection = transaction.selection;
  let targetSelection: Selection | null = null;
  if (currentSelection instanceof TextSelection) {
    const textSelection = currentSelection as TextSelection
    targetSelection = TextSelection.create(replicatedTransaction.doc, textSelection.$anchor.pos, textSelection.$head.pos);
  } else if (currentSelection instanceof AllSelection) {
    targetSelection = new AllSelection(replicatedTransaction.doc);
  } else if (currentSelection instanceof NodeSelection) {
    targetSelection = NodeSelection.create(replicatedTransaction.doc, currentSelection.$from.pos);
  }

  if (targetSelection) {
    replicatedTransaction.setSelection(targetSelection);
  }
// <-- end of snip -->

  DoNotProcessInBlockNodeDeletionPluginMetaKeys.forEach((key) => {
    const value = transaction.getMeta(key);
    if (value) {
      replicatedTransaction.setMeta(key, value);
    }
  });

  replicatedTransaction.setMeta(ProsemirrorTransactionMeta.REPLICATED_TRANSACTION, true);

  log('Applying replicated transaction')
  targetEditor.dispatch(replicatedTransaction);
  return true;
}

This is called in 'onTransaction' with the editor that was updated providing the sourceEditor EditorView and the editor receiving the update providing the targetEditor EditorView. transaction is the Transaction as received by the onTransaction event hook.

@GTarkin
Copy link

GTarkin commented Jul 24, 2024

Hi @nperez0111

my colleague has now investigated the issue and it seems the tiptap-code is ok and working good. The issue we experienced stemmed from a slighty changed invocation order of a watched field we had in our vuex-store and how the editors updated their internal state.

Before the fix you mentioned, the watcher fired after the editor state was already updated, now they fire before that has happend. This is the causes the invalid update of the selection.

We wrapped the offending piece of code within the watcher with a $nextTick and are fine now.
Thanks for your help!

@nperez0111
Copy link

@GTarkin , Appreciate the update, yea I was somewhat concerned about doing the editor updates before. But this is actually somewhat in line with how the react integration works too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Open Source The issue or pull reuqest is related to the open source packages of Tiptap. Type: Bug The issue or pullrequest is related to a bug
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.