re-frame-10x icon indicating copy to clipboard operation
re-frame-10x copied to clipboard

Performance improvements - remove garden/spade?

Open kimo-k opened this issue 1 year ago • 7 comments

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

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 avatar Jan 18 '24 18:01 kimo-k

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.

kimo-k avatar Jan 18 '24 18:01 kimo-k

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!

beders avatar Jan 20 '24 06:01 beders

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 avatar Jan 20 '24 15:01 kimo-k

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.

kimo-k avatar Feb 01 '24 20:02 kimo-k

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

beders avatar Feb 02 '24 01:02 beders

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

kimo-k avatar Feb 02 '24 01:02 kimo-k

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).

beders avatar Feb 22 '24 01:02 beders