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

Post styling perf 2019 09 17 #32716

Conversation

@pkozlowski-opensource
Copy link
Member

commented Sep 17, 2019

This PR removes unnecessary DOM reads (classList / style) in the most common case of using a procedural renderer. This simple change removes the most expensive function(s) (self time) from the profiller's radar.

Before:

setClass before

After:

setClass after

Before this refactoring native node `classList` / `style` properties were
read even if not used. The reason for this was desire to avoid code duplication
between procedural and non-procedural renderers. Unfortunatelly for the case
which will be used by most users (a procedura renderer) the `classList` / `style`
properties were read twice, making the `setStyle` \ `setClass` functions the
most expensive ones (self time) in several benchmarks (large table, expanding
rows).

This refactoring adds a bit of code duplication in order to get better
runtime performance. The code duplication will be removed when we drop
checks for a procedural renderer.
@googlebot googlebot added the cla: yes label Sep 17, 2019
@ngbot ngbot bot added this to the needsTriage milestone Sep 17, 2019
@pkozlowski-opensource pkozlowski-opensource marked this pull request as ready for review Sep 17, 2019
@pkozlowski-opensource pkozlowski-opensource requested review from IgorMinar and angular/fw-core as code owners Sep 17, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

FYI, VE and Ivy presubmits look normal. Thank you.

AndrewKushnir added a commit that referenced this pull request Sep 17, 2019
Before this refactoring native node `classList` / `style` properties were
read even if not used. The reason for this was desire to avoid code duplication
between procedural and non-procedural renderers. Unfortunatelly for the case
which will be used by most users (a procedura renderer) the `classList` / `style`
properties were read twice, making the `setStyle` \ `setClass` functions the
most expensive ones (self time) in several benchmarks (large table, expanding
rows).

This refactoring adds a bit of code duplication in order to get better
runtime performance. The code duplication will be removed when we drop
checks for a procedural renderer.

PR Close #32716
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
…ar#32716)

Before this refactoring native node `classList` / `style` properties were
read even if not used. The reason for this was desire to avoid code duplication
between procedural and non-procedural renderers. Unfortunatelly for the case
which will be used by most users (a procedura renderer) the `classList` / `style`
properties were read twice, making the `setStyle` \ `setClass` functions the
most expensive ones (self time) in several benchmarks (large table, expanding
rows).

This refactoring adds a bit of code duplication in order to get better
runtime performance. The code duplication will be removed when we drop
checks for a procedural renderer.

PR Close angular#32716
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

commented Oct 18, 2019

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 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.