day8 / re-frame-10x

A debugging dashboard for re-frame. X-ray vision as tooling.
MIT License
632 stars 68 forks source link

Performance improvements - remove garden/spade? #409

Open kimo-k opened 8 months ago

kimo-k commented 8 months ago

Discussed in https://github.com/day8/re-frame-10x/discussions/408

Originally posted by **beders** January 2, 2024 Hi there, first of all: thanks for re-frame-10x. Very useful. That said, team adoption is hindered by performance issues. With the `subs` tab open, typing into any input field is so slow that it becomes unusable. Even with the `subs` tap not showing, performance is affected quite significantly. Use Chrome's Profiler I think I found one reason for 10xs slowness: Garden CSS. Here's a snapshot of how much time is actually spent on compiling, expanding and setting CSS. ![image](https://github.com/day8/re-frame-10x/assets/522318/5c0bd681-9c50-484c-916b-abc0e561bde2) For some traces, it seems re-com is responsible for a lot of new CSS creation on every render, but it seems 10x is using it directly as well. So my proposal is to use something like TailwindCSS or another standard static CSS library to avoid the performance hit or maybe figure out why `garden.do_compile` runs for every keystroke (i.e. re-render)
kimo-k commented 8 months ago

Hey @beders, thanks for the feedback. We've experienced subjective slowness from spade but haven't bothered to look into it so far.

I can reproduce what you're seeing, and I agree - it's not nice.

Recent changes to spade (https://github.com/dhleong/spade/pull/17) add an optimization that seems like it would fix this. But after upgrading spade, I still see just as many css compiler calls - even on static classes like flex-style.

I've done some re-com prototypes using shadow-css. If a quick solution doesn't appear, then think I'll try replacing spade with shadow-css.

beders commented 8 months ago

Thanks so much for looking into this. We have a lot of subscriptions in our app and every ms saved when re-rendering that screen helps!

kimo-k commented 8 months ago

Yeah, no prob. It's not obvious to me what causes the compile to happen. Maybe spade doesn't properly memoize its behavior because we're rendering in a shadow root? A minimal repro would be really useful. Let me know if you get any insight, otherwise I'll keep poking around periodically.

kimo-k commented 8 months ago

Hey @beders, I took out all the spade code used by the subs tab and replaced it with plain css. You can try it out in version 1.9.5. Eventually I think I'll remove spade entirely & try for another clojure-friendly way to organize our styles.

beders commented 8 months ago

Sweet! I'll give it a go in one of our largest apps. THANK YOU!

kimo-k commented 8 months ago

cool. actually, make sure to use 1.9.6, though. there were some styling bugs in the earlier release.

beders commented 7 months ago

I finally had time for a bit of testing. I've tested 1.9.8 which shows some improvements.

image

The trace here shows a few keystrokes in an input field in our app (with over 281 subs) with the Subs tab open. A key stroke event recomputes 34 subs (78 are ignored). I guess the underlying issue here is: Don't have 281 subs ;)

Thanks for the great improvement. Looks like re-com.box is spending some time in initialization (clojure map destructuring is expensive) and merging.

Digging a bit deeper, there's some spade code that contributes 100ms to this trace:

image

Not sure where to go from here. Turning on Highlight updates when components render in the react dev tools shows a lot of repaints in the re-frame-10x window (I can't tell how many of those 281 subs are repainting), so maybe there's a way to reduce updates. For now this is much faster than before (I can't compare the numbers directly since I migrated to a different machine in the meantime).