perf(platform-browser): disable styles of removed components instead of removing#64524
perf(platform-browser): disable styles of removed components instead of removing#64524mattlewis92 wants to merge 1 commit intoangular:mainfrom
Conversation
| */ | ||
| interface UsageRecord<T> { | ||
| elements: T[]; | ||
| elements: Map</** Host */ Node, /** Style Node */ T>; |
There was a problem hiding this comment.
Why do we need a Map here?
There was a problem hiding this comment.
It was the data structure that was ported from the original commit that was reverted:
What would you propose instead, just a plain object?
There was a problem hiding this comment.
This code has been refactored greatly over the past year or so, and from what I am seeing elements: T[]; can be retained as the data structure as mostly I am seeing elements.values() which makes using a Map redundant here.
There was a problem hiding this comment.
What about here when we remove the host?
angular/packages/platform-browser/src/dom/shared_styles_host.ts
Lines 259 to 261 in 39bb013
There was a problem hiding this comment.
the removeHost is used used only in the ShadowDomRenderer, and in this case both this.inline and this.external will not be populated for the host that was added via addHostas addStyles and removeStyles are not used for shadow dom.
There was a problem hiding this comment.
Makes sense, thank you! Fixed in 035ae4a (#64524)
9ee205d to
035ae4a
Compare
|
@mattlewis92 This will likely have an impact on the new |
Sure I can test that - do you have any kind of animations playground setup / reproduction of the previous issue where I can test it against? |
thePunderWoman
left a comment
There was a problem hiding this comment.
This looks good to me. I think since you're re-using existing codepaths, this shouldn't have any impact on the animations use case. So we should be good.
|
I'll take care of running a TGP over the weekend. |
|
TGP is failing. |
Would it make sense to rename |
|
@thePunderWoman was the internal usage of the |
035ae4a to
637603b
Compare
|
@mattlewis92 I'm unaware of the addStyles cleanup issue. Is there an open issue associated? |
Just basing it on this comment from above:
|
|
@mattlewis92 Ah, I see. No, to my knowledge that has not changed. |
637603b to
62118f1
Compare
|
You might have only run the node test targets and the not browser test targets ? |
|
OK I think it's fixed now - the remaining test passes on my local machine, so I guess it's flakey? |
|
This one is flakey yes. |
|
Looks like the presubmit passed! 🚀 |
|
I'm seeing some slight changes on some screenshot tests but I wouldn't consider them minor. No sure they're flakey. |
|
Anything you're able to share to help me debug it? (I'm in the angular slack if you want to DM it to keep it private) |
|
Had a quick chat with Matt, this is probably a breaking change as the order of the styles elements might be different with this change. Can you update the commit and the PR to reflect that Matt ? Thanks |
|
Done! |
|
The CI failure looks unrelated to this change, it wasn't failing on Friday, and I don't think this PR added more than 5kb of code to exceed the bundle budget threshold |
b973f10 to
a84a102
Compare
bc293de to
e71b31a
Compare
|
@mattlewis92 Quick update: the reason for the delayed review is that @dgp1130 has been working on a style encapsulation PR for a while, and I was worried this would throw a wrench into his work. Once his stuff has landed, I can take a look. |
|
We'll need a rebase after the latest changes we did on the SharedStylesHost. |
…of removing When a component is destroyed, instead of removing its style elements from the DOM, we now disable them. This avoids expensive style recalculations triggered by DOM removal while still preventing the styles from applying. When the same component is re-created, the styles are re-enabled.
9cf2d51 to
589e24c
Compare
Done! |
| // no-op. This is necessary because `BaseAnimationRenderer.destroy()` defers | ||
| // `delegate.destroy()` via `afterFlushAnimationsDone` + `queueMicrotask`. | ||
| // Without this, the deferred `disableStyles()` would find the *replacement* | ||
| // component's newly-added style record (same content key) and disable it. |
There was a problem hiding this comment.
Can we add a TODO to clean this up after the legacy animation package is removed?
This commit changes the behaviour of
REMOVE_STYLES_ON_COMPONENT_DESTROY.Now,
styleandlinknodes are disabled instead of removed from DOM which avoiding the browser having to reparse the stylesheet when the stylesheet is re-enabled.This was previously reverted in 1640743 but it seems like the observed perf regression in chromium has since been fixed in https://issues.chromium.org/issues/40911913, so it should be possible to reland this perf improvement
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
When all components using a stylesheet are destroyed, the stylesheet is removed from the DOM
Issue Number: 59651
What is the new behavior?
The stylesheet is now marked as
disabled, which improves performance the next time the stylesheet is enabled, as the browser will not have to incur the cost of reparsing the stylesheet when enabling it (although it will still trigger style recalculations)Does this PR introduce a breaking change?
When a component is destroyed, its associated
<style>elements are nowdisabled in the DOM rather than removed. When the same component is recreated, the styles
are re-enabled at their original position instead of being re-appended to the end of
<head>. This may change the CSS cascade order of component stylesheets, which couldaffect styling if your application relies on the order in which component styles appear
in the DOM.
To migrate, avoid depending on the insertion order of Angular component stylesheets in
<head>. If you have styles that must take precedence over component styles, use morespecific CSS selectors or increase specificity rather than relying on source order.
Applications that do not rely on stylesheet ordering in
<head>are not affected.Other information