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

Perf: decorations propagated like selectors #4997

Conversation

jasonphillips
Copy link
Contributor

@jasonphillips jasonphillips commented May 18, 2022

Description
This is an alternative proposal to reconcile the ongoing debate over decorations in #4993 - I haven't fully analyzed everything here just yet but it seems to work very well.

Instead of sending context updates down the react tree to all components for the decorate function, this approach follows more closely the pattern of useSlateSelector (update: refactored both to use the new official useSyncExternalStoreWithSelector hook) which means that it does not require additonal components in between, and simply registers listeners that use the decoration range comparison to check for a change before choosing to rerender.

In my initial testing, this is indeed a little bit faster than the old, purely context-driven approach to decorate--particularly as the document grows in size--due to this listener style being generally faster than context alone for sending changes down a large tree (which is why other libraries use it).

At the moment this still follows the assumption that you only redecorate if the function changes or if the node is part of a change. However, it is absolutely trivial on this approach to set some other condition for re-computing all decorations even if the function is constant (just have to invoke onDecorateChange). So I think we could add some sort of API change at the level of the Editable to easily accomodate both styles here (always redecorating everything, or only when function or node changes).

So I'd like to solicit feedback on how we might do that; I'm not sure what shape that should take so that it isn't awkard and fits the general style of slate's current props and options.

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 May 18, 2022

🦋 Changeset detected

Latest commit: 63c277c

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

This PR includes changesets to release 2 packages
Name Type
slate-react Patch
slate Patch

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

@jasonphillips jasonphillips changed the title Fix: decorations propagated like selectors Perf: decorations propagated like selectors May 18, 2022
@jasonphillips
Copy link
Contributor Author

jasonphillips commented May 18, 2022

I've updated this PR to switch over from the older redux style of implementing selectors to the newer useSyncExternalStore() hooks provided officially as a shim and in later react versions; the implementation is much simpler, and I carried it over to the useSlateSelector hook as well -- as was suggested back in #4841 (comment)

@dylans
Copy link
Collaborator

dylans commented May 18, 2022

My day slipped away, but overall I think this looks promising. I'll find time in the next day or so to look and to also give feedback on your question about the API for specifying the decoration approach.

@dylans
Copy link
Collaborator

dylans commented May 20, 2022

Thinking about this some more, iiuc this uses both context and passing props down below the context. I'm far from an expert on React, and this may be fine, but I've generally seen one approach or the other. I'm also making sense of the feedback in #4993 (comment) .

With respect to a configuration, #4999 has some similar questions for a different problem where they have calls that don't rely on Editor. I don't think that's an issue here and providing a boolean on the Editor seems simplest assuming we make this configurable.

@jasonphillips
Copy link
Contributor Author

iiuc this uses both context and passing props down below the context

I can certainly answer this part.

In this case the context isn’t used at all for sending updates, it’s only for initial subscription when a component mounts—like redux etc; they don’t send updates through context either and use it only to subscribe to the store.

In general context is not very efficient for broadcasting updates to many descendants in a large structure, which is why libraries tend to take this approach. (As a side note: I think it would be a very good move to eliminate the other expensive use of context in slate in the future, which is the selection context. It adds a lot of overhead to each keystroke. But that would be a breaking change since the exposed useSelected hook would have to change signature.)

As for the question about the ideal API, I’m not necessarily in favor of just having a Boolean that changes how decorations behave... it feels like some extra pattern should be considered to better accommodate the annotations-like needs of cursors etc for their specific use cases, without switching to a mode where all decorations are always recomputed on the entire tree. In other words, both use cases can easily live in the same project.

@bryanph
Copy link
Contributor

bryanph commented May 27, 2022

@jasonphillips you're using the use-sync-external-store approach here to resolve the problem that React currently has with not having a way to avoid unnecessary context-triggered re-renders right? So once that becomes available we can switch to something simpler (useContextSelector)? Overall this approach makes sense to me, nice job on this PR 👍

@jasonphillips
Copy link
Contributor Author

@jasonphillips you're using the use-sync-external-store approach here to resolve the problem that React currently has with not having a way to avoid unnecessary context-triggered re-renders right? So once that becomes available we can switch to something simpler (useContextSelector)? Overall this approach makes sense to me, nice job on this PR 👍

Basically correct, although I would say that there are 2 issues with react contexts. The most pressing issue for performance that you referenced here is that we can't only rerender when some part of the context value or derived value from it changes, and as a consequence all elements in the tree currently rerender when you change the decorate function instead of only those where the decorations would be changed. That problem would be solved if they ever release an official context selector API.

The other problem, though, is that react contexts are still very poorly optimized for broadcasting changes to a large number of descendants. This is another reason that libraries like redux, apollo-client, etc all use the useSyncExternalStore method now to derive state from a store that is merely subscribed on mount via context, but where the context itself never changes, and some sort of listeners instead broadcast the changes.

@jasonphillips jasonphillips force-pushed the fix/decorations-selection-style branch 2 times, most recently from 19a0abc to dff1cb9 Compare May 30, 2022 22:26
@ShellOliver
Copy link

I was looking for something like that to solve some performance issues that I have with decorators. What is preventing it to be merged?

@dylans
Copy link
Collaborator

dylans commented Aug 8, 2022

I was looking for something like that to solve some performance issues that I have with decorators. What is preventing it to be merged?

We started by reverting the change that had caused the initial performance regression and decided to see if that was reasonable or if more work needed to be done. Are you still experiencing issues with Slate 0.82?

@jasonphillips
Copy link
Contributor Author

I was looking for something like that to solve some performance issues that I have with decorators. What is preventing it to be merged?

Are your performance issues happening in particular when changing the decorate function? I believe that's the main place where you'd see improvement here, since currently it is the case that replacing the function causes all nodes to rerender, but this PR makes it so that you only rerender nodes whose resulting decorations changed.

@ShellOliver
Copy link

Are your performance issues happening in particular when changing the decorate function? I believe that's the main place where you'd see improvement here, since currently it is the case that replacing the function causes all nodes to rerender, but this PR makes it so that you only rerender nodes whose resulting decorations changed.

Exactly, I have a decorator that changes very frequently and it affects only a few components but it should not be rerendering everything. This PR is an improvement over the existing functionality.

@jasonphillips
Copy link
Contributor Author

Exactly, I have a decorator that changes very frequently and it affects only a few components but it should not be rerendering everything. This PR is an improvement over the existing functionality.

Okay thanks, that makes sense (out of curiosity, did you get a chance to try it out on your real app using this PR's code?). I can try to rebase and look back over this PR within the week to make it more fresh, because I think it's worth keeping, but I just haven't returned to look at the issue in a while.

@ShellOliver
Copy link

Okay thanks, that makes sense (out of curiosity, did you get a chance to try it out on your real app using this PR's code?). I can try to rebase and look back over this PR within the week to make it more fresh, because I think it's worth keeping, but I just haven't returned to look at the issue in a while.

I just tried it in my app and it works! before that I also were able to compare this and the main with this decorator in the richtext example:

...
const [flag, setFlag] = useState(false)

  const customDecorate = useCallback(
    ([node, path]) => {
      if (node.type === 'block-quote' && flag) {
        const range = {
          highlighted: true,
          anchor: { path, offset: 0 },
          focus: { path, offset: 1 },
        }

        return [range]
      }
      return []
    },
    [flag]
  )

  return (
    <Slate editor={editor} value={initialValue}>
      <Toolbar>
        <button onClick={() => setFlag(!flag)}>toggle</button>
  ...

I added heading-two at the schema example and a console.log into it; and another console.log on the block-quote the unique one that should be rendered when clicking on the toggle button that I added in the toolbar. The console fromheading-two shows in the main branch whenever I click on this toggle but does not show on this PR. I got the same behavior in my application but with real decorators.

@jordanpapaleo
Copy link

jordanpapaleo commented Aug 10, 2022

@jasonphillips Thanks for this PR. My team is super interested in this change. Is there anything you need assistance with?

@jasonphillips jasonphillips force-pushed the fix/decorations-selection-style branch from dff1cb9 to 7a518a6 Compare August 11, 2022 22:00
@jasonphillips
Copy link
Contributor Author

Okay, I've rebased this and have deployed it on a version of our own application in the meantime to test a bit further.

@jasonphillips Thanks for this PR. My team is super interested in this change. Is there anything you need assistance with?

I guess the main thing would be to test with this code if you're able to push a patched / prerelease version of slate-react into your use case, and ensure that it is working well for you.

I can definitely confirm that the standard "search highlighting" example works much better with this code, due to the fact that changing the decorate function no longer causes all nodes to rerender. But this PR also refactors the critical useSlateSelector() implementation, and while I've used a similar syncExternalStore pattern in my own application in production it's still worth us testing as much as possible.

Since there was a bit of unnecessary back and forth after the last time decorator logic was changed, I don't want to cause any new debate, so let's make sure everyone agrees 👍

@nemanja-tosic
Copy link
Contributor

In general I've a couple of things i would ideally like to see with this PR, which i did in the following commits that we could cherry-pick if they look useful. I'm going to explain the changes in each. I'm still experimenting with the possibility to produce decorators so that no custom equality check is needed.

Get decorations in Text nodes only (and some refactors)

Roughly:

  • given that a context exists, there should be no need to pass decorations down the tree, we should be able to get them in Text nodes only
  • onDecorateChange should be moved to the useDecorateStore, there is no need for editable to use it
  • some TypeScript refactoring to add explicit types, especially on boundaries

Changes: 2c484b4

Update useSyncExternalStore usage

Roughly:

  • use the no selector format recommended by the react team - really not sure if worth it, but my reasoning is that there is no selector version in the react 18 api, so i figured it's better to comply
  • performance optimizations for calculating decorations - Node.levels instead of Node.nodes to traverse tree

Changes: e2b8294

@jasonphillips
Copy link
Contributor Author

jasonphillips commented Oct 4, 2022

@nemanja-tosic

I think this helps to push things forward quite a bit, thanks. I was letting this PR stagnate, partly because the decorations topic is so tricky and it's hard to know what use cases everyone has in their application--so I didn't want to force any changes until we have a chance for many of us to react to this proposal.

My initial concern is only about this part:

  • given that a context exists, there should be no need to pass decorations down the tree, we should be able to get them in Text nodes only

I think this makes sense as a design, but I'm worried that this won't work for some use cases that currently are working fine on slate.

If I'm reading the change correctly: instead of computing decorations at each element of the tree and passing them down to children, this would have each leaf (text) node be the only place where decorate() is called--but since decorations can be placed on higher-order elements or ranges, each of the leaf nodes here also calls decorate() on all of its parent elements up the tree, then checks those for intersections.

So if you have decorations added at a higher level--for example, a decoration on the editor itself to render some kind of user cursor, or a decoration on an element that is a syntax-highlighted block containing many text children--then the decorate function for those parents will be called on each child individually, and the intersection checked at each child individually as well. If you have a structure like SyntaxHighlightedContainer -> CodeLine[400] -> Text, and you decorate at the level of the SyntaxHighlightedContainer since the code lines depend on their context and since it's expensive to do the highlighting, then each leaf of that structure will call decorate() on the parent here indivdually. In this example, each leaf will call decorate on the SyntaxHighlightedContainer, so that's 400+ calls to the same thing.

Ideally if the application developer knows this, they would memoize with a weakmap or something--but right now, you don't have to cache it because decorate isn't called repeatedly on an element for each child. This seems to make it a breaking change if I'm reading correctly. Perhaps we should create a weakmap on the slate side, per-render, something like CURRENT_RENDER_TO_DECORATED_NODE, so that there isn't any repetition and the client application doesn't have to think about caching on the other end. I might try that.


So -- then I read your second commit after typing the above response, and it looks like you do use a WeakMap : )

That looks like it helps to reduce the calls, but that cache is per leaf node--so_ won't we still have the situation above when first decorating a large block with children? Meaning, we'll be redundantly calling the parent decorate for every leaf, which in some cases might be hundreds or thousands. Forgive me if I misread the code here, but shouldn't we also ensure that we don't call decorate on the parent element repeatedly for every leaf on initial render?

Perhaps the ideal WeakMap here would actually just be NODE_TO_DECORATIONS at the top rather than per leaf, and where this weakmap is discarded and recreated every time decorate() changes.

@nemanja-tosic
Copy link
Contributor

e2b8294 is not focused on performance improvements, rather on getting decorations to with the stock useSyncExternalStore, which does not have the memoization parameters like the shim. I'm not sure if it's "a must" but it feels like conforming to the interface the library exposes might be better, given that it isn't that much work.

I've roughly measured the performance for the changes I've introduced compared to what was there before. The unit tests are unchanged too, so the same number of calls to decorate remains. I'd need to look at the code again, but as far as i remember the same number of intersections are called too, as the leaves only change on element changes (invalidating the entire tree) or decorate changes (invalidating the entire tree).

That being said, i'm sure I've not covered all the cases and i'm probably misremembering or misunderstanding something, so if i could ask you to add a couple of the cases you mentioned as failing unit tests it would help me try to improve the code, and the same specs could prevent slate from regressing in the future? If you could commit here, i'll cherry pick and figure out if my idea on "purity" (using contexts and not props) is invalidated by performance issues it introduces.

@MattIPv4
Copy link

👋 I've been running into a lot of lag when a large number of decorations are rendered, and was pointed in the direction of this PR as a potential solution -- what's the latest on the progress here, is it likely to be landed? Would it be possible to get this rebased so we can try it out?

@jasonphillips
Copy link
Contributor Author

@MattIPv4 I'll find some time for this over the next few days, to try and more fully evaluate the performance piece by adding additional tests @nemanja-tosic suggested above. I feel that we're close to a solution everyone can agree on, so long as we just verify there aren't additional calls happening here for use cases where the tree is deep and has decorations added at higher levels.

@jasonphillips jasonphillips force-pushed the fix/decorations-selection-style branch from c3923b6 to 63c277c Compare January 22, 2023 01:31
@jasonphillips
Copy link
Contributor Author

I rebased and also added a better test to slate-react for decorations, which uses nested blocks so that performance issues show up: 63c277c

We should have 6 decorate() calls for one render:

  • editor [ ]
    • parent block [0]
      • block [0,0]
        • text [0,0,0]
      • block [0,1]
        • text [0,1,0]

With the suggested changes from above, we get 9 calls on this trivial example:

  • editor [ ]
  • when rendering the first leaf text, it calls decorate on all parents up the tree:
    • editor [ ] (again)
    • parent of parent block at [0]
    • parent block at [0,0]
    • text at [0,0,0]
  • then the last leaf does the same, repeating:
    • editor [ ] (3rd time)
    • parent of parent block at [0] (2nd time)
    • parent block at [0,1]
    • text at [0,1,0]

So I do like those changes in some ways, but calling & caching decorate only from the bottom upward like this (each leaf calls decorate on its parents) explodes the number of calls on any kind of nested structure. If you have something even more nested like editor -> section-block -> code-block -> code-line -> leaf, you'd see a major degradation.

Calling decorate from the leaf like that also means checking Range.intersect() against every decoration higher in the tree for every leaf. So if you have a decoration added to the editor itself at the top level, every single leaf in the editor with check its intersection against that decoration whenever a leaf re-renders--instead of only checking at the top level of elements and never sending that decoration down the tree further into blocks that it doesn't intersect.

@jasonphillips
Copy link
Contributor Author

Maybe there's a way to get the best of both approaches... caching decorate as suggested, but instead of caching it per-leaf we'd need it to be cached per entire render of the editor. If that was done, it's possible that the leaf-based approach (call decorate on all parents when a leaf renders) would work without nested structures causing so much repeated work.

If I'm thinking correctly, it's mostly only the initial render (when all leaves are re-rendered at once) which will lead to way too many repeated decorate calls for nested structures if using the leaf approach, because after that point we're usually only re-rendering one part of the tree anyway.

@jasonphillips
Copy link
Contributor Author

One more problem with the leaf-based logic that I realized when testing a bit: if we were to only redecorate from the leaf level (meaning when the leaf rerenders, calls decorate on its ancestor nodes and checks for intersection, suggested in ), there are use cases which work on current Slate but would be broken.

For instance, say you have a container and children, nested like code-container { code-line, code-line, code-line, ... }. You might add decorations for syntax highlighting on the container block, because it has the full content to work with and therefore it can handle syntax highlighting that bridges multiple lines. So when one child is edited, and the tree up above it rerenders, new decorations would be applied to the container which might affect all the children, so multiple children might need to rerender as a consequence. That works on the old code, because any time the container gets new decorations, it invalidates its children if the changed decorations would intersect those children.

But with the approach of decorating from the leaf level, only the changed child blocks would update their decorations, and even though the container gets new decorations that should affect other children, they wouldn't be applied.

For this reason and those mentioned, I'm inclined to suggest that we just review & merge this PR's implementation first, and not try the more radical restructuring of 2c484b4. Another PR could try that, but it implies some breaking changes for certain situations which currently work, and some changes to the performance that are different from expected (nested blocks suddenly made much less efficient to decorate, etc).

That said, I don't expect this PR to speed up decorations by a whole lot, but they do seem (anecdotally) a little bit faster in my testing on our own large application.

@MattIPv4
Copy link

MattIPv4 commented Jan 23, 2023

So I pulled this down locally to test it out (thank you for rebasing!), and I've noticed quite a few times that this seems to characters being typed out of order compared to key presses, or even extra characters being typed at points.

It seems that when a decoration range is being applied, sometimes the undecorated text also gets left behind in the DOM? My understanding of Slate perhaps isn't good enough to diagnose exactly what is happening, but when these ghost characters get left behind selecting them and changing their text appears to affect the correctly decorated text, so Slate seems to have some awareness of them, but obviously not in the right way.

Perhaps this is due to our logic calculating decorations in a denounced manner? We wait for about 30ms of no input change before we update our decorations -- which can result in you typing a word, and it then being decorated after, which is potentially what is causing these ghost characters here? The code is unfortunately private.

@jasonphillips
Copy link
Contributor Author

Perhaps this is due to our logic calculating decorations in a denounced manner? We wait for about 30ms of no input change before we update our decorations -- which can result in you typing a word, and it then being decorated after, which is potentially what is causing these ghost characters here? The code is unfortunately private.

I see, perhaps we can get a minimal reproduction of just that part, the debounced decorations.

How do you debounce it? Do you perhaps update the top-level decorate() function after a delay from last change? It's definitely worth looking to see what's going on, particularly if your use case was working on the current release without these changes.

@MattIPv4
Copy link

We debounce with a setTimeout based on the serialized value of the editor -- and then yeah, we recalculate all our decorations and update the top-level decorate function such that it returns the new decorations (those decorations are grouped by node, we don't return any decorations for the editor itself).

@jasonphillips
Copy link
Contributor Author

We debounce with a setTimeout based on the serialized value of the editor -- and then yeah, we recalculate all our decorations and update the top-level decorate function such that it returns the new decorations (those decorations are grouped by node, we don't return any decorations for the editor itself).

In theory, replacing the decorate() function from the outside at any time shouldn't be a problem, since that's what the search-highlighting example does and it works fine. I can't seem to reproduce a situation like you describe when I try many different variations locally... it seems that we may need some simplified code to test it out.

@jasonphillips
Copy link
Contributor Author

We debounce with a setTimeout based on the serialized value of the editor -- and then yeah, we recalculate all our decorations and update the top-level decorate function such that it returns the new decorations (those decorations are grouped by node, we don't return any decorations for the editor itself).

Just to be certain, I'd also recommend verifying against the very latest slate version (without the PR changes) if you weren't already on the very latest. It's at least possible that some other change is involved.

@MattIPv4
Copy link

I am running the latest version of Slate -- I'll try to put together a minimal reproduction using this branch later this week

@jasonphillips
Copy link
Contributor Author

jasonphillips commented Jan 24, 2023

@MattIPv4

Just thinking of possible causes...

Could there be any place in your code where you mutate decoration objects instead of using them immutably? A performance improvement in this PR reduces the cloning of decorations down the tree because Range.intersection() now will return the left-hand range if the intersection is equal to the whole range--so if there happened to be any case in your code where you modify a decoration object in-place rather than creating a new one, that could cause some consequences.

Alternatively, do you make any particular uses of Range.intersection() in your application code where you might mutate something it returns (instead of spreading it into a new object, etc)?

@MattIPv4
Copy link

👀 We are very careful to not mutate stuff ever, tends to cause unexpected things in the world of React. Hopefully I can get a minimal reproduction going for you to take a look at

@MattIPv4
Copy link

MattIPv4 commented Jan 27, 2023

Here's a reproduction from the plain-text example:

https://gist.github.com/MattIPv4/e1e576f208387d9ef361750206b852ef

  1. Click to place the cursor at the end of the editor, wait briefly, observe all decorated as bold
    (bug: cursor jumps back to start)
  2. Type a at the end of the editor, wait briefly, observe all decorated as bold
  3. Type a again at the end of the editor, wait briefly, observe a decorated a appear, as well as the plain-text a persisting
    (bug: plain-text a persisting)
  4. Type a again at the end of the editor, wait briefly, observe internal Slate error
    (bug: internal Slate state seems to be broken)

(As a note on 3. here, if you look at console to see what is being decorated, Slate seems to only know about two as at this point, even though three as are rendered in the DOM)

@jasonphillips
Copy link
Contributor Author

@MattIPv4

Actually, that example helps tremendously and you are entirely right -- a use case like yours (which should be valid) causes a problem with the store approach.

And it's not easy to solve, to be honest. We use a useSyncExternalStore() approach similar to this PR in part of my own employer's large codebase with Slate and it operates great, but we tie the store's flush (or "broadcast" to listeners) to be synchronized with an editor onChange in every case... which sadly can't be done with the way decorate has been designed in slate-react's current design, as a direct prop on the Editable element.

If it were hoisted and changed from being an invalidated prop to some other API (eg. letting the user call onDecorateChange() to recalculate decorations) we could get more performance and use the store reliably. But the decorate-prop setup leaves us with a bit of a delicate house of cards, because the Editable rerendering has to be perfectly aligned with the external decorate changes for slate's timing on reconciling selection etc to account for all cases.

I also tried the alternative suggestions above (the leaf-based approach) since that has some nice cleanup for the external-store implementation... and it still has the same problem with your example.

We may have to cancel this concept of a major refactor, and go with an even simpler PR just to patch up a few little things for performance as best as possible. It's difficult to adjust this setup as-is without breaking something for an existing use case out there.

@MattIPv4
Copy link

Apologies for finding this! ❤️

I agree that an API call to let Slate know you've redecorated would probably make sense here, but I'm not sure it'd be worth the breaking change of removing the prop (unless there's a world where the prop can still exist and the API call can be added too). I leave that call with you and others.

If this change is a no-go though, due to this, hopefully some small performance improvements can still be landed! Every little bit helps as they say, especially when you have a lot of decorations.

@dylans
Copy link
Collaborator

dylans commented Dec 16, 2023

"We may have to cancel this concept of a major refactor, and go with an even simpler PR just to patch up a few little things for performance as best as possible. It's difficult to adjust this setup as-is without breaking something for an existing use case out there."

We are slowly starting to write tests for slate-react which could help.

@jasonphillips do you have interest in that simpler PR (or did you do it already and I've already forgotten about it)?

For now I'm going to close this larger PR as it's grown rather stale.

@dylans dylans closed this Dec 16, 2023
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.

None yet

7 participants