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

A solution for reflex-dom + jsaddle-warp UI slowness, stutters, freezes. #224

Open
Wizek opened this issue Apr 13, 2018 · 4 comments
Open

Comments

@Wizek
Copy link

Wizek commented Apr 13, 2018

Hello @ryantrinkle. I've thought to reproduce here most of the email that I've sent you a about 10 days ago since others might be affected by this issue as well, and might benefit from me mentioning it here. And @hamishmack might also be interested to be included since it's related to jsaddle{,-warp}. (Even though we might decide to make these changes entirely on the level of reflex-dom without any changes required from jsaddle.)

I've remembered that you were saying that this sync UI stutter/lag/lockup issue was only a problem with jsaddle-warp, since you think that it is much less efficient to go through the HTTP layer than the tighter integration that jsaddle-webkit2gtk and the like can do.

So after having investigated the segfault issue regarding building reflex-dom with webkit even with nix, and having been able to overcome it briefly, I've found that I'm experiencing similar level of UX degradation there as well. Which to me wasn't very surprising since my rough measurements also indicated about 4ms overhead per sync call here too.

And I also thought further that even if I was able to build a near-perfect version with webkitgtk, I still want to make sure that the jsaddle-warp version is working to my satisfaction as well.
Why? Because I find it very portable, e.g. being able to be built and/or used on Windows, with stack/nix, with/without WSL. I'm trying to retain this flexibility for supporting many platforms, and the flexibility to build with simpler dependencies too. I'm trying to avoid painting myself in the corner where I'll only be able to build with nix, and cannot run on Windows.

Also, a friend of mine is actually using the app remotely every now and again. Yes, we know that jsaddle-warp was only ever meant to serve on localhost, but he finds that for now that ~100ms latency is still better since he then at least is able to use the app than not having any access.

So here we are nearing some good news.

You may remember that last time I was flabbergasted [5] by the amount and kinds of sync calls that were required even for rendering, even for mousemove event handling. I set out to try to eliminate them and see how that turns out. First I thought I could do it on my application level, re-defining some reflex event handlers. But that turned out to not work well/at all since I had no way to disable reflex's sync render call at each frame. [5] Therefore I went in and changed that part of reflex to become async. [1] Then another idea hit me: What if instead of me fiddling and changing 5-20+ relevant call sites I could go lower and just turn all sync calls to be async? And so I've found a quite narrow code path in jsaddle itself that allowed me to do just that. [2]

After waiting for approximately 1 (one) eternity for jsaddle-dom to compile, to my surprise the app built and worked quite close to perfectly. There were only a few places where I was relying on preventDefault that misbehaved. These, I thought, I should be able to move over to the JS side without much hassle. [3]

After having changed these too in my app it works quite close to flawlessly now -- I only needed to touch 3 call sites. Gone are the UI stutters/lockups/freezes! This also completely fixes the jsaddle(-warp?) bug where it can go into an endless XHR loop freeze. Everything goes through async websocket packets with these changes. In fact, this jsaddle freeze bug was the one that pushed me over the edge because this made the UI close to being impossible to use for me.

An aside note:
I also had an initial worry that events might get re-shuffled and processed out of order, but then I realized that WS is built on top of TCP, so packet order is actually guaranteed by that layer in both directions! I did notice a few times some weirdness related to mousemove x/y coordinates being strangely handled, seemingly processed out of order, but not sure how that would be possible over TCP. A competing theory of mine is that what I am observing is not reorder, but effects of variable latency. Sometimes the event processing seem to lag behind a bit. I haven't profiled or investigated where this sometimes-increased latency comes in, could be the HS side, or WS messaging, or JS rendering. My suspicion is on the JS rendering side. And if that turns out to be the case some kind of frame dropping/throttling could be a nice solution: my app doesn’t need to draw each mousemove event. The first and the last one is important, but a few in-between can be dropped without any issues.

But this aside is all only for future consideration, because in practice this doesn't seem to be much of an issue whatsoever. The UI is so much more responsive that this apparent glitch is a very small issue in comparison. So it’s a no-brainer to go forward in this direction as far as I am concerned. Another happy side effect is that the friend of mine will be able to use the app remotely even better since the UI won’t lock up for hundreds of milliseconds at a time, just lag behind a bit. Much better UX.

And now I find myself at a bit of a crossroad and hence I am reaching out to you. The two commits that I’ve made to reflex-dom and jsaddle that I’ve mentioned above are only for proof of concept, for me to figure out if this experiment could turn out successful at all. Successful it did, and now I wonder how we could solve this more idiomatically, more permanently, potentially someplace upstream. E.g. could we do this on the level of jsaddle or reflex-dom? Which one would make the most sense? Would we want to make this a fundamental change for all reflex code, or would we like to allow app developers to decide if they’d like the sync functionality? E.g. Until we investigate that apparent out-of-order glitch. Or in very narrow cases for preventDefault/stopPropagation that can be still necessary. (I personally found that this is not a problem since I can still use preventDefault with a bit of JS. [3])

I’d be interested in discussing this briefly or more deeply either in text (IRC (Wizek)/email/GitHub issue) or in a voice call. Are you interested as well?

Best regards, and looking forward to hearing from you,
Milan Nagy

P.s. If you might find the above unconvincing on just how much of a difference these small changes make, I’m open to showing the difference to you.

P.p.s If you wish to see a place where these changes are used all together, then you may be interested in this repo and branch [4] that I’ve used for experimentation. Beware of messy code.

@Wizek Wizek changed the title A solution for reflex-dom +jsaddle-warp UI slowness, stutter, freezes. A solution for reflex-dom + jsaddle-warp UI slowness, stutters, freezes. Apr 13, 2018
@ryantrinkle
Copy link
Member

@Wizek I'm not quite willing to break preventDefault. However, @hamishmack and I have been working on a major refactor of the jsaddle core to address these issues. The relevant branches are named things like experimental-core. It's not quite ready yet, but I think it will be fairly soon.

Apologies for the slow response, and I must admit it may be a little while before I'm able to read everything you've written here and give it the consideration it needs. Hopefully we can get everything in a state that works for you as well as all the other users of jsaddle soon!

@Wizek
Copy link
Author

Wizek commented Apr 14, 2018

Apologies for the slow response, and I must admit it may be a little while before I'm able to read everything you've written here and give it the consideration it needs.

Glad to read even this brief reply. Looking forward to your more detailed feedback.

I'm not quite willing to break preventDefault

Maybe this will become clearer upon a more thorough reading, but I don't think I'm proposing that either. My apps usually rely on preventDefault too. It's just a slightly different way to achieve the same behavior: [3]. And in all cases, I'm not even saying we'd have to apply this minor async-limitation to all code, just that it could make sense to make this configurable from within reflex-dom or jsaddle* somehow, so those app developers who suffer from UI slowness/stutters/freezes can try it out and see if it helps them.

hamishmack and I have been working on a major refactor of the jsaddle core to address these issues

Great! Maybe/hopefully that will also address all these problems I've experienced and I can switch over. But I'm not holding my breath if it'll still rely on sync XHRs, and not fully (or mostly) async WS messages as the POC above shows, as there seem to be some fundamental UX problems with the sync XHR approach.

@Fresheyeball
Copy link

@ryantrinkle any progress here?

@tek
Copy link
Contributor

tek commented Apr 22, 2021

@Wizek could you explain that gist where you add the event using JSA.call a bit? I'm getting a requirement for ToJSVal (RawElement (DomBuilderSpace m)) since my element is created outside of the prerender thunk in which I'm trying to use your code and I don't know how to satisfy this sensibly. Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants