-
Notifications
You must be signed in to change notification settings - Fork 50
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
Lose extra diff #167
Comments
This is much much too slow! About 35ms too slow. Thanks for flagging it. Why do you suspect the patchCallback would be faster? |
I have been doing some more experimenting with this and a couple of things came to light. Firstly, diff works much faster on newer documents with fewer changes. This makes sense as diff moves through all changes in the doc to create patches. However, the patchCallback doesn't seem to take significant time at all. My understanding of this is that patches are effectively produced during each change to progress the document at the js level. They are effectively already produced, so little additional work is required. This was reflected in my tests too. |
Okay, yeah, I seem to remember @orionz warning me about this. I'd moved to explicit diffs in anticipation of wanting to support branching / shutting back and forth on the timeline. I guess I'd underestimated the cost. We should fix this. |
I'd like to get this fixed, but I could do with a little guidance. Change events are emitted by the However, patches don't really feel like they belong in the state machine to me? The alternative would be to emit changes outside of the state machine, and only use xstate to track the internal state (loading, ready etc). What do you think? |
Yeah patches feel like they shouldn't be part of the state machine, which is more concerned with the availability of the document. Can we have a |
That almost feels as though it is still part of the state. To my mind patches only apply to the change they are emitted with |
I agree that it's awkward to hold the state on the We currently don't have to track this "last patched heads" state anywhere because the |
That's really what I was proposing in the first solution above. I'll knock up a prototype and see if it works for now. |
Closing this out -- we've instead made the extra diff basically free. |
While not massive in the grand scheme of things, it is roughly 1/3rd of the time for a change which causes a render (the dropHandler in the flamechart) and significantly more (>70%) for the two small changes which follow. |
We are currently generating change patches by calling
Automerge.diff
after each change. For an average change, thisdiff
takes ~35ms.While this isn't a spectacularly long period of time, the rest of the change can sometimes only take a single digit ms amount of time. If we could somehow get the patches from the
patchCallback
, this would likely speed up the whole transaction.We could also get the change source at the same time.
The text was updated successfully, but these errors were encountered: