improve performance of style tags that use precedence #32957
+108
−8
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RJWadley
force-pushed
the
precedence-perf
branch
from
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
|
Andarist
reviewed
Apr 22, 2025
Comment on lines
+4527
to
+4535
| 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.