Skip to content
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.

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

perf(platform-browser): disable styles of removed components instead of removing #51808

Closed

Conversation

alan-agius4
Copy link
Contributor

@alan-agius4 alan-agius4 commented Sep 18, 2023

This commit changes the behaviour of REMOVE_STYLES_ON_COMPONENT_DESTROY.
Now, style nodes are disabled instead of removed from DOM. This causes the same runtime behaviour but avoids recomputations when the stylesheet is re-added when the component is re-created. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/css_style_sheet.h;l=266;drc=31fb07c05718d671d96c227855bfe97af9e3fb20

NB: This changes is being done following some performance bottlenecks observed in Phanteon and their own recommendations.

Context:
http://chat/room/AAAAxKxTk40/jaP6Lj6fhmQ/jaP6Lj6fhmQ
https://crbug.com/1444522
http://b/289992821

@alan-agius4 alan-agius4 added area: core Issues related to the framework runtime action: presubmit The PR is in need of a google3 presubmit core: performance target: minor This PR is targeted for the next minor release labels Sep 18, 2023
@ngbot ngbot bot added this to the Backlog milestone Sep 18, 2023
@alan-agius4 alan-agius4 added target: patch This PR is targeted for the next patch release and removed target: minor This PR is targeted for the next minor release labels Sep 18, 2023
@alan-agius4 alan-agius4 marked this pull request as ready for review September 18, 2023 11:17
@alan-agius4 alan-agius4 added the action: review The PR is still awaiting reviews from at least one requested reviewer label Sep 18, 2023
@alan-agius4 alan-agius4 force-pushed the perf-dom-renderer-remove-css branch 6 times, most recently from e3d9560 to f40b24a Compare September 18, 2023 15:39
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

@alan-agius4 thanks for the improvement 👍

packages/platform-browser/src/dom/shared_styles_host.ts Outdated Show resolved Hide resolved
@alan-agius4 alan-agius4 force-pushed the perf-dom-renderer-remove-css branch 3 times, most recently from 5e9fe7f to cc84960 Compare September 19, 2023 08:11
…of removing

This commit changes the behaviour of `REMOVE_STYLES_ON_COMPONENT_DESTROY`.

Now, `style` nodes are disabled instead of removed from DOM. This causes the same runtime behaviour but avoids recomputations when the stylesheet is re-added when the component is re-created. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/css_style_sheet.h;l=266;drc=31fb07c05718d671d96c227855bfe97af9e3fb20

NB: This changes is being done following some performance bottlenecks observed in Phanteon and their own recommendations.

Context:
http://chat/room/AAAAxKxTk40/jaP6Lj6fhmQ/jaP6Lj6fhmQ
https://crbug.com/1444522
http://b/289992821
@alan-agius4
Copy link
Contributor Author

@AndrewKushnir, as mentioned offline, I had to re-add CSS removal/disabling for components that use view encapsulation. This is because a number of components in G3 use ::ng-deep which causes the styles to escape encapsulation and causes a number of screen test targets to break.

@alan-agius4
Copy link
Contributor Author

alan-agius4 commented Sep 20, 2023

TGP
TGP Deflake

NOTE: TGP is green after deflaking.

@alan-agius4 alan-agius4 removed the action: presubmit The PR is in need of a google3 presubmit label Sep 20, 2023
@alan-agius4 alan-agius4 removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Sep 21, 2023
@alan-agius4 alan-agius4 added action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Sep 21, 2023
@dylhunn
Copy link
Contributor

dylhunn commented Sep 22, 2023

This PR was merged into the repository by commit 3c0577f.

@dylhunn dylhunn closed this in 3c0577f Sep 22, 2023
@alan-agius4 alan-agius4 deleted the perf-dom-renderer-remove-css branch September 22, 2023 18:58
mattlewis92 pushed a commit to mattlewis92/angular that referenced this pull request Oct 5, 2023
…of removing (angular#51808)

This commit changes the behaviour of `REMOVE_STYLES_ON_COMPONENT_DESTROY`.

Now, `style` nodes are disabled instead of removed from DOM. This causes the same runtime behaviour but avoids recomputations when the stylesheet is re-added when the component is re-created. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/css_style_sheet.h;l=266;drc=31fb07c05718d671d96c227855bfe97af9e3fb20

NB: This changes is being done following some performance bottlenecks observed in Phanteon and their own recommendations.

Context:
http://chat/room/AAAAxKxTk40/jaP6Lj6fhmQ/jaP6Lj6fhmQ
https://crbug.com/1444522
http://b/289992821

PR Close angular#51808
alan-agius4 added a commit to alan-agius4/angular that referenced this pull request Oct 17, 2023
alan-agius4 added a commit to alan-agius4/angular that referenced this pull request Oct 17, 2023
…instead of removing (angular#51808)"

This reverts commit 65786b2 due to an oberved perf regression in Pantheon.

See: http://b/303667704
pkozlowski-opensource pushed a commit that referenced this pull request Oct 18, 2023
…instead of removing (#51808)" (#52238)

This reverts commit 65786b2 due to an oberved perf regression in Pantheon.

See: http://b/303667704

PR Close #52238
pkozlowski-opensource pushed a commit that referenced this pull request Oct 18, 2023
…instead of removing (#51808)" (#52238)

This reverts commit 65786b2 due to an oberved perf regression in Pantheon.

See: http://b/303667704

PR Close #52238
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 23, 2023
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
…of removing (angular#51808)

This commit changes the behaviour of `REMOVE_STYLES_ON_COMPONENT_DESTROY`.

Now, `style` nodes are disabled instead of removed from DOM. This causes the same runtime behaviour but avoids recomputations when the stylesheet is re-added when the component is re-created. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/css_style_sheet.h;l=266;drc=31fb07c05718d671d96c227855bfe97af9e3fb20

NB: This changes is being done following some performance bottlenecks observed in Phanteon and their own recommendations.

Context:
http://chat/room/AAAAxKxTk40/jaP6Lj6fhmQ/jaP6Lj6fhmQ
https://crbug.com/1444522
http://b/289992821

PR Close angular#51808
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime core: performance target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants