improve performance of style tags that use precedence #32957
Conversation
fe605aa to
c200aa6
Compare
|
Comparing: bc6184d...7c917f5 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
| function appendStyleRule(style: HTMLStyleElement, cssText: string) { | ||
| try { | ||
| if (style.sheet) | ||
| style.sheet.insertRule(cssText, style.sheet.cssRules.length); | ||
| else style.textContent += cssText; | ||
| } catch (e) { | ||
| style.textContent += cssText; | ||
| } | ||
| } |
There was a problem hiding this comment.
Please take my feedback with a grain of salt. I'm not a React team member - but we were chatting with @gnoff about most of this already.
I think this change should be reverted (for now). I totally agree this is something that will have to be implemented in some capacity at some point but there are open design questions around this.
It feels like, at the moment, this breaks the core assumption that a single resource is represented by a single element. OTOH, not many tests were affected by this change and that makes me doubt a little bit in my interpretation of the change.
All in all, I'd focus first on caching insertion points for each precedence to avoid expensive and redundant querying.
There was a problem hiding this comment.
I would be interested to see react manage these styles better. perhaps using insertRule and deleteRule to remove/update styles rather than inserting them into the DOM forever. that would be beyond the scope of this PR, of course, but it bothers me that react makes no attempt to clean up these styles when components are unmounted.
I could imagine many cases where a component gets mounted & unmounted in a cycle, and that would have React adding & removing the styles in a cycle as well, wasting CPU time. Leaving styles forever is not a bad option.
There was a problem hiding this comment.
yeah, that's fair - it's definitely faster at first to leave them, but it gets slower with time if you don't clean up.
my main issue with leaving styles in is it causes headaches in development.
imagine you write this during development:
<style href="sample" precedence="low">
{" h1 { color: red; } "}
</style>
but nah, you've changed your mind. red isn't your color. you replace it:
<style href="sample" precedence="low">
{" h1 { color: blue; } "}
</style>
what color do you think paragraphs will be now? If you've read the docs and know a bit about how CSS works you probably know. I find this to be expected, but a bit unintuitive and definitely annoying.
There was a problem hiding this comment.
This is one case where dev should behave differently than prod, imo. Dev should probably insert/remove whole style tags at once, removing old ones. Prod should probably be optimized for an append-only use-case, possibly with insertRule.
The deleteRule API is also annoying to use, it uses a rule index rather than a rule id, so managing rules is not an O(1) operation once the style rules array becomes holey (or in other words, when you need to delete rules in the middle of the stylesheet). I don't think it's particularly well suited for high-perf.
There was a problem hiding this comment.
Wouldn't it be possible to insert/remove only when a hot reload cycle has just passed, and for the other interactions, retain the deduplication behavior.
For instance, in react-refresh runtime, one could keep a global key for the whole app that gets regenerated every time any module with components hot reloads. Associate this key with each injected styles. When deduping on href/precendce, compare the styles key with the global one to know if they should be recreated or deduped.
Summary
Currently, every time we insert a new style tag that has precedence, we first need to run 2 querySelectorAll calls. If we want to render 400 style tags, we'll need to run querySelectorAll 800 times. This is fairly slow.
Additionally, every time we encounter a new style tag react will create a new style tag in the DOM, and will (as far as I can tell) make no attempt to remove that style tag at any point. So the process of inserting itself will get slower as we insert more and more styles.
We can address the performance in this specific case by inserting nodes less often:
a few other random notes
fixes #32806
fixes #30739
How did you test this change?
I've built this version of react and patched it into a few projects that were having performance issues. I'm planning on doing this again (I've tweaked a few things) and will update these results when I've done that.
in one such expensive render:
the entire process (including the browsers style recalculation) went from 2.11s to 504.34ms
the react portion of that process went from 1.47s to 26.91ms
flushMutationEffects (the previously slow part) went from 1.45s to 7.58ms
in one more extreme case:
the entire process (including the browsers style recalculation) went from 9.57s to 600.18ms
the react portion of that process went from 8.63s to 41.05ms
flushMutationEffects (the previously slow part) went from 8.61s to 6.67ms
These two examples are obviously not fully comprehensive (I could fabricate a fairly comprehensive example case if anyone is interested) but they do roughly represent the level of improvement I've seen in my testing.