Reduce number of component updates

[mattkrick]

Do you want to request a feature or report a bug?

Bug

What's the current behavior?

Render time is slow.

In the HugeDocument example, a keypress event takes ~300ms (CPU slowed down 6x)

Compare that to the exact same document, but without updates propagated through react. It's <100ms:

Proposal:

(Re)introduce selective rendering logic.

As I understand it, here's the current flow:

1. key event
2. add OT to the queue
3. flush the OT queue
4. update the component via setState
5. react updates DOM (EXPENSIVE)
6. reset scroll & selection range via cDU (EXPENSIVE)

Here's an improved flow:

1. key event
2. add OT to the queue. Each OT that has a behavior that is different than the native DOM content editable behavior sets a 1-way pushUpdate flag
3. flush the OT queue
4. update the component via setState
5. use shouldComponentUpdate = () => value.get(pushUpdate)
6. If update, do steps 5 & 6 above

Key notes:

• An enhanced solution is to delay flushing the OT queue until it's required (eg a request to serialize), similar to what immutable-js does. I'm ruling this out of scope to keep it simple.
• This is a blocker for Android support RFC: Fix for Android devices #1935.

[ianstormtaylor]

Hey @mattkrick, thanks for opening this and for presenting a potential solution.

Could you explain/decouple the solution for Android from the performance issue? I’m curious if there are other solutions that might allow for Android support, without needing to use shouldComponentUpdate to short circuit the rendering process, since doing so brings many negatives.

[ianstormtaylor]

Ah, just saw the #1935 thread. For more context in that case as to why I’d like to not use shouldComponentUpdate...

Slate is unlike some other React components in that it exists as a layer between your components, not strictly under them in the rendering tree. Because of this, using shouldComponentUpdate is problematic, since it short circuits the tree, leaving your leaf components out of the loop as to when changes happen.

For example someone may want to change the color of a paragraph when the text content changes. Or they may want to change color based on how far away the cursor is from a specific node. These are just examples, but the point is that the logic required for shpuldComponentUpdate blocking is often more restrictive than the logic you want to render with.

And last I checked there wasn’t a good way to be able to block rendering in the middle of the tree, but have it continue again further down a specific branch. Maybe the new stuff coming out from fiber eliminates this issue?

So that’s why I’m curious if there are other solutions we can make use of.

[mattkrick]

there are certainly ways to re-render children even when their parent has shouldComponentUpdate return false. I think that should be out of scope for this PR, but I think it's a great idea to leave that door open, so I'd propose an API like this: pushUpdate: boolean | BlockKey | Array<BlockKey>. That way, in the future, a plugin could tell slate whether to rerender everything (pushUpdate = true), rerender a single block (pushUpdate = '123') or rerender a couple blocks (pushUpdate = ['123', '456']).

For example someone may want to change the color of a paragraph when the text content changes.

this is a great example! Let's roll with it:

1. I wrote a changeColorOnDiff plugin.
2. I notice crummy performance, like what you see above.
3. I add in a simple method:

class ChangeColorOnDiff {
  shouldSlateUpdate() {
    return oldColor != newColor
  }
}

if it's a legacy plugin or the author is lazy & doesn't include shouldSlateUpdate, then it returns true by default. Worst case scenario, the performance is no worse than it is today! The key difference is that it's no longer slate's fault for slow renders, it's the plugin's fault. That means when jerks write issues about poor performance (like this one) you can tell them to go bother the plugin author.

Internally, it's just an itty bitty bit of code:

const defaultUpdater = () => true
let pushUpdate = []
for (const plugin of plugins) {
  const shouldSlateUpdate = plugin.shouldSlateUpdate || defaultUpdater
  const newUpdates = shouldSlateUpdate(argsTBD)
  if (newUpdates === true) {
    pushUpdate = true
  } else if (Array.isArray(newUpdates) || typeof newUpdates === 'string') {
    // future proof API
    pushUpdate = pushUpdate.concat(newUpdates)
  } else {
    throw new Error('invalid return val')
  }
  // short circuit future plugin `shouldSlateUpdate` calls for extra performance
  if (pushUpdate === true) break
}

[ianstormtaylor]

@mattkrick I think what you're describing is already possible with shouldNodeComponentUpdate for a node's component, but I might not be understanding exactly.

there are certainly ways to re-render children even when their parent has shouldComponentUpdate return false

Can you explain this?

[mattkrick]

Ah shoot, lemme try to explain it better. I'll break it down in 3 different ways, lemme know if I make sense with any of em.

By far, the most expensive operation is step 6:

reset scroll & selection range via cDU (EXPENSIVE)

shouldNodeComponentUpdate is not useful because it can't prevent that. it comes far too late. it comes after you've already told react to start rendering the entire editor. It's the equivalent of pruning a branch. I'm proposing we cut down the whole darn tree.

---

Another way to think about it is a controlled vs. uncontrolled component. Today, it is a controlled component. You trigger an input, the input simultaneously updates the HTML element (cheap!) as well as slate's internal store. Then, it throws away the calculated HTML element value & calculate a new HTML element based on slate's internal store (expensive!). Just like an uncontrolled input, we'd let the HTML do its default thing, and then only when we need the value do we ask the DOM for it.

---

This proposal is to insert a logic gate to decide whether to choose the cheap pre-computed HTML element, or the expensive calculated HTML element.

Let's use a real example.

• I have a basic editor with no plugins.
• I hit the "a" key
• The HTML Element receives an "a" and the caret moves 1 position to the right (this is the default onInput behavior for a DOM content editable).
• Slate's after#onInput handler beautifully creates an operation that adds an "a" and moves the caret 1 position to the right. it then applies that operation to its store
• At this point, we know with 100% certainty that If we rendered slates internal store into a component, it would look exactly like the precomputed HTML element. So why bother?

Now, let's add plugins.

• I have an editor with a plugin that changes the color when there are an odd number of "a"s in the text
• I hit the "a" key
• The HTML Element receives an "a" and the caret moves 1 position to the right (this is the default onInput behavior for a DOM content editable).
• The plugin counts up the "a"s, and creates an operation to change the color. it tells us not to trust the default behavior (pushUpdate == true).
• Slate's after#onInput handler creates an operation that adds an "a" and moves the caret 1 position to the right. it then applies both operations to its store
• At this point, we assume that If we rendered slates internal store into a component, it would look DIFFERENT than it does right now (because the plugin told us so). So scrap the cheap default calculation & render the expensive one based on slate's internal store.

---

To answer your question about shouldComponentUpdate, you can do anything that forces a child to think. eg introduce their own sCU, add a controller like redux's connect, change internal component state with a callback, fire an event emitter that the component listens to & then calls forceUpdate, have the component subscribe to an observable, etc.

[ianstormtaylor]

Hey @mattkrick thanks for expanding.

The issue I see is that in your example, the plugin is affecting the “data” by adding its own operation to the queue to change the color of a node. But what I’m trying to account for is not changing color with an operation itself, but purely as a rendering concern.

To give another example, imagine a small counter next to every paragraph that showed the word count of the block. It’s not something that adds operations, it simply renders extra information. If the editor decided to use shouldComponentUpdate to abort rendering because it was a simple text insertion, then the node component doesn’t get the chance to re-render it’s word counter.

[mattkrick]

great example!

so let's break it down & call it data for all the blocks (ie the stuff that gets serialized) and metadata for the extra info (word count, etc).

rendering metadata is cheap, and it's already the plugin's fault for being slow at rendering it, so we don't care about improving that. rendering data is the expensive piece.

to keep it cheap, we need to know when to invalidate props.children in the renderEditor method.
if pushUpdate == false, then we shouldn't have to rerender it.
that means "reset scroll & selection range via cDU" logic needs to be moved down the tree.

So the parent component always renders, but the data component (the piece that returns props.children to the plugin's renderEditor) will have sCU and cDU logic on it.

Now, if the plugin is super advanced & it takes props.children & mutates it, that's fine. that plugin will just cause a rerender on every operation (it's the plugin's job to return pushUpdates = false if render cycles can be saved. the default is always to do expensive rerenders).

[ianstormtaylor]

@mattkrick I think we're not talking about the same thing, which may be my fault for not being clear. Here's a JSFiddle showing a Paragraph node component which determines its color based on its text length: https://jsfiddle.net/24r81abc/11/

As far as I can tell, this would not be possible if shouldComponentUpdate restricted the re-rendering tree to no have the node re-render.

[ianstormtaylor]

Or, similarly, here's one that styled nodes based on the current selection: https://jsfiddle.net/fj9dvhom/1224/. Again I don't think would be possible if the editor could short-circuit the rendering tree.

[mattkrick]

oh wow that 2nd one is fun!

i'd expect the logic rules for that plugin to look like this (sorry for the pseudo code):

let pushUpdate = false;
if (oldAnchorBlockKey !== anchorBlockKey || oldFocusBlockKey !== focusBlockKey || oldBlocks.length !== blocks.length) {
  pushUpdate = true
}
return pushUpdate

in other words, if you type the letter "a", no update necessary. if you hit a backspace & you're not at the beginning of the line, no update necessary. But if you click to a different line, or you hit the Enter key, or you paste a bunch of lines of text, yeah, you gotta update.

[ianstormtaylor]

@mattkrick maybe, but that just forces the user to end up writing and managing incredibly complex diffing logic themselves, when most of the time it isn't a bottleneck. I'm going to close this. Feel free to open new issues with specific actionable items for which parts of rendering are slow and could be avoided.

[mattkrick]

maybe, but that just forces the user to end up writing and managing incredibly complex diffing logic themselves

perhaps i didn't make this clear. the onus is on the plugin author. if they don't want to write that logic, that's fine. The performance is no worse than it is today.

Feel free to open new issues with specific actionable items for which parts of rendering are slow and could be avoided.

I listed the exact bottlenecks in the original comment:

5. react updates DOM (EXPENSIVE)
6. reset scroll & selection range via cDU (EXPENSIVE)

I've detailed out a plan to avoid those bottlenecks without the need for a breaking change. I don't know what else I can do.

[ianstormtaylor]

@mattkrick fair enough, I'll keep this open for discussion. Here are some additional thoughts...

It's the equivalent of pruning a branch. I'm proposing we cut down the whole darn tree.

That's exactly what worries me.

I think the "branch pruning" approach is the one we need to strive for if we want to do something like this. The problem with the "cut down the whole tree" approach is that it has huge collateral damage, which results in leaky abstractions. If a single plugin aborts rendering the entire editor, there's no way of guaranteeing that another plugin didn't depend on the editor being re-rendered with its new state.

This could technically be solved by some sort of communication abilities between plugins, but it gets very complex. (At least as far as I can understand from what you've described.)

I listed the exact bottlenecks in the original comment:

It's hard to really tell where the performance issues are from your descriptions, because those flamegraphs don't really give much information...

5. react updates DOM (EXPENSIVE)

Maybe so, but the render logic in Slate contains calls to tens or hundreds of methods, any one of which would be where most of the time spent rendering is. I haven't seen any guarantee that the biggest performance issue is React's DOM updating logic, and not some slower method in Slate's internals that should be optimized instead.

If anything, I'd think that React's DOM updating logic runs in constant time for inserting text on small or large documents as long as the paragraphs themselves are the same size, and that it's much more likely that some of the methods in Slate's core whose run time is based on tree size would be the culprits.

6. reset scroll & selection range via cDU (EXPENSIVE)

Again, maybe so. I believe this one slightly more. But before nuking the entire rendering approach, it merits investigate what exactly is slow, and whether it's something that is avoidable via other means. I'd guess that there is more than one way to render and updated selection.

Put simply, the "cut down the entire tree" approach is a huge change to make with many places it can be a leaky abstraction to the layers above it. For that reason, we'd be much better off doing almost any other performance improvements that are restricted to the data layer that might have gains.

---

Based on everything you've described though—again thank you for opening this for discussion—I think there are potential areas for optimization, using the "branch pruning" approach.

Since Slate's component hierarchy looks like this in pseudo-code:

<UsersApp> (userland)
  <Editor>
    <Content>
      <Node>
        <UsersNode> (userland)
          <Text>
            <Leaf>
      <Node>...
      <Node>...

There are issues with aborting rendering at the <Editor> level, since the <UsersNode> component level never fires. Right now we already have the shouldNodeComponentUpdate which essentially allows userland components to abort at the <Node> level when they need to.

But as you mention, there's no easy way to abort early enough to stop the DOM from needing to be updated.

We could potentially achieve this with logic that was able to abort at the <Text> or <Leaf> level, preventing re-rendering for the simple cases of insert/remove which are 90+% of Slate operations in normal editing cases.

These could even be done internally, without needing any userland plugins or other logic which can be complex for them to maintain. Operations could potentially contain some extra data about nodes that have had native updates and should not be re-rendered. (As opposed to containing only the nodes that should be re-rendered.)

But, like I mentioned before, this is only one potential optimization. And I don't think there's any guarantee yet that it's the most important one to make to improve performance for large documents.

If you or anyone else wants to investigate and do some in depth research with numbers/code to back up the assertions I'd welcome it.

[isubasti]

@ianstormtaylor I think the performance of keypress described on this issue was due to the example site was using the dev version of react which has been solved by #1939 . The current performance of keypress with production build is now ~40ms instead of ~300ms. Also tested production build of #1975 with memoization on slate react seems to reduce it down even further to ~20ms.

[dylans]

I think this one has improved a lot over the past 3-4 years of work, so I'm closing it out.