Slate: Undo stack implementation retains unreachable `Value` objects, leaks memory

[bengotow]

Do you want to request a _feature_ or report a _bug_?

Bug

What's the current behavior?

Open the https://www.slatejs.org/#/history slate example, open the Developer Tools and start a Memory > Allocation Instrumentation on Timeline profile. Then perform the following:

1. Type 4-5 characters
2. Undo the typed characters so the undo stack length is 0
3. Type a character to clear the redo stack
4. Type another character to remove any remnants of previous props from React's memoziedProps.

Observe that memory allocated when the original 4-5 characters were typed is still around, even though it is now impossible to reach those operations via undo/redo:

• Note: I've verified that disabling undo/redo entirely and it also fixes these leaked allocations, they're definitely undo/redo related.

What's the expected behavior?

Slate should not retain memory related to undo/redo operations which can no longer be reached via undo/redo.

Fix

I think this is caused by the fact that the undo redo stack is stored in value.data. Since each undo operation has a copy of value, and value contains an undo stack, Slate creates a recursive structure of Value's with undo stacks that reference older values, and keeping a handle to a single undo/redo entry is enough to retain the entire stack forever. Clipping to 100 undo entries is nice, but since entry 0's operation.value.data.get('undos') retains 100 more entries further into the history, it's not effective at limiting the memory footprint.

After applyOperation applies an undo or redo, the code restores the current undo/redo stack which means that it actually doesn't need the undo/redo stacks of each saved Value at all. I patched Commands.save to remove these and it resolves the issue:

Commands.save = (editor, operation) => {
  const { operations, value } = editor

  // Remove the undo/redo history from the operation's Value
  operation = operation.withMutations(op => {
    const v2 = op.value.withMutations(v => {
      let d2 = op.value.data
      d2 = d2.remove('undos')
      d2 = d2.remove('redos')
      v.set('data', d2)
    })
    op.set('value', v2)
  })

  ...

@ianstormtaylor I think my hack above is fairly gross, so I've filed this as an issue and not a PR just yet. I actually don't know much about ImmutableJS and I'm not sure if there's a better way to accomplish this, or if another fix would be preferable?

[joeferner]

I had to do a slightly different hack. Different versions?

operation.withMutations(o => {
  const properties = o.get('properties');
  if (properties && properties.data) {
    properties.data = properties.data.remove('undos').remove('redos');
    o.set('properties', properties);
  }
}).toJS()

[bengotow]

Ahh maybe so? That looks nice though with the immutable operations chained. I was testing on latest master (0.44.6)

I put together a small test demonstrating this on top of the Slate history example. I just added an additional button that runs this code, appending small changes that modify but do not make the document longer.

  onBreakIt = () => {
    let counter = 0

    this.editor.moveToRangeOfDocument().moveToEnd()

    setInterval(() => {
      counter += 1
      this.editor
        .deleteBackward(17)
        .insertText(`Hello ${counter.toPrecision(10)}`)
    }, 1)
  }

Without the fix in place, the browser tab adds about 100MB to the Javascript heap for every 1000 undo stack entries, indefinitely. About 10,000 mutations in, the tab is using 1GB or RAM. 😬

I'm going to take your better looking ImmutableJS and make a PR - really want to get this fixed! I use Slate in the Mailspring email client (and really love this library), and people are reporting that spending a while composing an email results in the app using gigabytes of memory.

[zhujinxuan]

Hi, @ianstormtaylor Do we need to store value.documents in operation stacks? It seems we have not used value.document in undo/redo.

[ianstormtaylor]

Not sure what is specifically being used right now or not, can't remember.

That said, we've talked about the root cause of this issue in https://github.com/ianstormtaylor/slate/issues/2123 and I'd love to see us change to not need any of those .value, .node, etc. properties. That way we can solve the root cause instead of circumventing it.

[bryanph]

Just a reminder that there is actually a PR open that fixes this from a while ago: https://github.com/ianstormtaylor/slate/pull/2225. It removes the need to keep track of value and makes all ops invertible (when converting from and to JSON).

[ianstormtaylor]

Thank you @bryanph, I actually had forgotten that. That is great!

[bengotow]

Oh awesome! It'd be amazing if the ops were invertible and keeping the historical value objects wasn't necessary - I think that'd reduce the memory footprint of undo/redo a TON, especially for large documents like the ones in Mailspring (that can contain a lot of quoted text). It looks like there's a lot in-flight, if there's anything I can do to help let me know. Really looking forward to the improvements in #2225!

[ianstormtaylor]

This is solved now. Thank you @bryanph!