8000 improve performance of style tags that use precedence by RJWadley · Pull Request #32957 · facebook/react · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

improve performance of style tags that use precedence #32957

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+108 −8
Open

improve performance of style tags that use precedence #32957

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c200aa6
improve performance of style tags that use precedence
RJWadley Apr 18, 2025
7c917f5
handle suspended style tags, update precedence test
RJWadley Apr 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
8000
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 105 additions & 2 deletions packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor
@romgrk romgrk May 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
Expand Up @@ -4522,6 +4522,79 @@ function getScriptSelectorFromKey(key: string): string {
return 'script[async]' + key;
}

// for the sake of performance we want to use insertRule
// if that fails, fall back to appending textContent (which is slow)
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;
}
}
Comment on lines +4527 to +4535
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const styleNodesByPrecedence = new WeakMap<
HoistableRoot,
Map<string, HTMLStyleElement>,
>();
const styleNodesByHref = new WeakMap<
HoistableRoot,
Map<string, HTMLStyleElement>,
>();

function updateHrefCache(
hrefCache: Map<string, HTMLStyleElement>,
node: HTMLElement,
) {
const hrefs = node.dataset.href.split(' ');
for (let j = 0; j < hrefs.length; j++) {
const href = hrefs[j];
if (
typeof HTMLStyleElement !== 'undefined' &&
node instanceof HTMLStyleElement
)
hrefCache.set(href, node);
}
}

// when creating our caches, hydrate with data from the DOM
// this should only happen once per root
function getHydratedCaches(hoistableRoot: HoistableRoot) {
let rootHrefCache = styleNodesByHref.get(hoistableRoot);
let rootPrecedenceCache = styleNodesByPrecedence.get(hoistableRoot);

if (!rootHrefCache) {
rootHrefCache = new Map<string, HTMLStyleElement>();
styleNodesByHref.set(hoistableRoot, rootHrefCache);

const nodesWithHref = hoistableRoot.querySelectorAll('style[data-href]');
for (let i = 0; i < nodesWithHref.length; i++) {
updateHrefCache(rootHrefCache, nodesWithHref[i]);
}
}

if (!rootPrecedenceCache) {
rootPrecedenceCache = new Map<string, HTMLStyleElement>();
styleNodesByPrecedence.set(hoistableRoot, rootPrecedenceCache);

const nodesWithPrecedence = hoistableRoot.querySelectorAll(
'style[data-precedence]',
);
for (let i = 0; i < nodesWithPrecedence.length; i++) {
const node = nodesWithPrecedence[i];
const precedence = node.dataset.precedence;
if (
typeof HTMLStyleElement !== 'undefined' &&
node instanceof HTMLStyleElement
)
rootPrecedenceCache.set(precedence, node);
}
}

return {rootHrefCache, rootPrecedenceCache};
}

export function acquireResource(
hoistableRoot: HoistableRoot,
resource: Resource,
Expand All @@ -4533,11 +4606,38 @@ export function acquireResource(
case 'style': {
const qualifiedProps: StyleTagQualifyingProps = props;

// Attempt to hydrate instance from DOM
let instance: null | Instance = hoistableRoot.querySelector(
const {rootHrefCache, rootPrecedenceCache} =
getHydratedCaches(hoistableRoot);

// attempt to hydrate first from our href cache, then from the DOM
// this minimizes the number of times we query the DOM
let instance: null | Instance =
rootHrefCache.get(qualifiedProps.href) || null;

if (instance) {
resource.instance = instance;
markNodeAsHoistable(instance);
return instance;
}

// attempt to reuse an existing style node before creating a new one
// this minimizes the number of style nodes we create, which keeps query selectors faster
instance = rootPrecedenceCache.get(qualifiedProps.precedence) || null;
if (instance && instance.isConnected) {
if (typeof qualifiedProps.children === 'string')
appendStyleRule(instance, qualifiedProps.children);
if (!instance.dataset.href.includes(qualifiedProps.href))
instance.dataset.href += ` ${qualifiedProps.href}`;
resource.instance = instance;
return instance;
}

// if a style tag with the same href was server-rendered but then suspended, it may not be in our cache
instance = hoistableRoot.querySelector(
getStyleTagSelector(qualifiedProps.href),
);
if (instance) {
updateHrefCache(rootHrefCache, instance);
resource.instance = instance;
markNodeAsHoistable(instance);
return instance;
Expand All @@ -4557,6 +4657,9 @@ export function acquireResource(
insertStylesheet(instance, qualifiedProps.precedence, hoistableRoot);
resource.instance = instance;

rootPrecedenceCache.set(qualifiedProps.precedence, instance);
rootHrefCache.set(qualifiedProps.href, instance);

return instance;
}
case 'stylesheet': {
Expand Down
9 changes: 3 additions & 6 deletions packages/react-dom/src/__tests__/ReactDOMFloat-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8314,7 +8314,7 @@ background-color: green;
</html>,
);

// Client inserted style tags are not grouped together but can hydrate against a grouped set
// Client inserted style tags are grouped and can also hydrate against an existing grouped set
ReactDOMClient.hydrateRoot(
document,
<html>
Expand Down Expand Up @@ -8347,11 +8347,8 @@ background-color: green;
<style data-href="11 13 14" data-precedence="default">
111314
</style>
<style data-href="16" data-precedence="default">
16
</style>
<style data-href="17" data-precedence="default">
17
<style data-href="16 17" data-precedence="default">
1617
</style>
<style data-href="2 5" data-precedence="foo">
foo2foo5
Expand Down
Loading
0