tonsky / rum

Simple, decomplected, isomorphic HTML UI library for Clojure and ClojureScript
Eclipse Public License 1.0
1.8k stars 126 forks source link

Big performance regression between 0.11.5 and 0.12.3 #242

Closed jwr closed 3 years ago

jwr commented 3 years ago

I had to revert from 0.12.3 back to 0.11.5 because of a significant regression in performance, to the point of making the app unusable for some users. I tried to find where the time is spent, but couldn't.

These performance problems show up as the app "freezing" and the browser becoming completely unresponsive, or UI updates taking so long that the search text box drops entered characters.

I am not using anything later than 0.12.3, because of #235.

Changing rum version from 0.12.3 back to 0.11.5 makes the app responsive again.

roman01la commented 3 years ago

@jwr Could you please check in which version the regression first appeared for you?

jwr commented 3 years ago

I tried compiling and testing with a range of different rum versions, but it didn't really produce any more data: 0.11.5 works OK, and my app doesn't start with 0.12.0. Version 0.12.3 does have the performance problem, and anything later does not work because of #235.

roman01la commented 3 years ago

@jwr it's hard to tell w/o a repro case or at least perf profile dump, but I can assume that since Rum removed batching on animation frame this surfaced potentially unsafe place in your code presumably with an update loop, where something constantly re-renders UI via state updates.

jwr commented 3 years ago

Ok, after a fix to #235 made it to HEAD, I tested and did some profiling. Indeed, there is lots of re-rendering going on, which causes the observed big performance regression.

Now, I do not dispute that this should be fixed at the source — but this is a huge and complex app and implementing batching in all scenarios is non-obvious. This is not "dumb" re-rendering in a loop, it's re-rendering because application state meaningfully changed, and it changed because of database updates on the server, which went through the changefeed and affected the client-side state.

The previous approach in Rum seems to have provided a reasonably good general approach to batching. Are there strong reasons to drop it forever? Any chance it could come back?

roman01la commented 3 years ago

@jwr as discussed before, I've put 0.12.8-SNAPSHOT with the same render queue but slightly different mechanism to work around input issues. Please give it a try and let me know if it works for you.

jwr commented 3 years ago

0.12.8-SNAPSHOT works fantastically well. Compared to 0.12.7, my app remains responsive in the face of multiple updates.

roman01la commented 3 years ago

That's great! I'm gonna push 0.12.8 now

jwr commented 3 years ago

Thank you so much for helping out with this. It means I can now switch over from 0.11. Very happy about that :-)

roman01la commented 3 years ago

fixed in 0.12.8