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

Ng class perf one map #48433

Conversation

pkozlowski-opensource
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource commented Dec 10, 2022

This commit represents rewrite of the NgClass directive to address
severe performance problem (excessive DOM mutation). The modified
algorithm removes all the known performance clifs and has number of
desirable properties:

  • it is shorter and (arguably) easier to follow;
  • drops usage of existing differs thus limiting dependencies on other
    part of the code without increasing size of the directive;
  • doesn't degrade any other performance metrics.

algorithm notes

The NgClass directive uses the custom change detection algorithm for its inputs. The custom
algorithm is necessary since inputs are represented as complex object or arrays that need to be
deeply-compared.

This algorithm is perf-sensitive since NgClass is used very frequently and its poor performance
might negatively impact runtime performance of the entire change detection cycle. The design of
this algorithm is making sure that:

  • there is no unnecessary DOM manipulation (CSS classes are added / removed from the DOM only when
    needed), even if references to bound objects change;
  • there is no memory allocation if nothing changes (even relatively modest memory allocation
    during the change detection cycle can result in GC pauses for some of the CD cycles).

The algorithm works by iterating over the set of bound classes, staring with [class] binding and
then going over [ngClass] binding. For each CSS class name:

  • check if it was seen before (this information is tracked in the state map) and if its value
    changed;
  • mark it as "touched" - names that are not marked are not present in the latest set of binding
    and we can remove such class name from the internal data structures;

After iteration over all the CSS class names we've got data structure with all the information
necessary to synchronize changes to the DOM - it is enough to iterate over the state map, flush
changes to the DOM and reset internal data structures so those are ready for the next change
detection cycle.

This was referenced Dec 10, 2022
@pkozlowski-opensource pkozlowski-opensource force-pushed the ng_class_perf_one_map branch 2 times, most recently from 3eeca64 to 81782fa Compare December 12, 2022 15:29
@pkozlowski-opensource pkozlowski-opensource added state: blocked area: common Issues related to APIs in the @angular/common package labels Dec 12, 2022
@ngbot ngbot bot modified the milestone: Backlog Dec 12, 2022
@pkozlowski-opensource
Copy link
Member Author

Putting is as "blocked" since there are 3 knows tests in G3 that are failing because of this PR. This is mostly due to the fact that those tests assert CSS class order: it is not guaranteed and changed in a subtle way.

Investigating to decide if I should mimic the previous order or update the existing tests.

@pkozlowski-opensource pkozlowski-opensource force-pushed the ng_class_perf_one_map branch 2 times, most recently from f93d01f to 910aaf2 Compare December 13, 2022 10:34
@pkozlowski-opensource pkozlowski-opensource marked this pull request as ready for review December 14, 2022 11:41
@pkozlowski-opensource pkozlowski-opensource added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Dec 14, 2022
@pkozlowski-opensource
Copy link
Member Author

Note: this PR will require G3 changes before landing (there are 3 failing tests that need to be updated). Having said this, the NgClass implementation seems correct and is ready for review.

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.

@pkozlowski-opensource great work! 👍

The changes are looking good and easy to follow. Extra thanks for documenting the approach and the algorithm!

I've left a few minor comments, none of them are blockers :)

packages/common/src/directives/ng_class.ts Show resolved Hide resolved
packages/common/src/directives/ng_class.ts Outdated Show resolved Hide resolved
packages/common/src/directives/ng_class.ts Outdated Show resolved Hide resolved
packages/common/src/directives/ng_class.ts Show resolved Hide resolved
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.

Reviewed-for: size-tracking

@pkozlowski-opensource pkozlowski-opensource force-pushed the ng_class_perf_one_map branch 2 times, most recently from 6a4579d to c286199 Compare January 2, 2023 12:38
@pkozlowski-opensource pkozlowski-opensource added the action: global presubmit The PR is in need of a google3 global presubmit label Jan 3, 2023
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.

Looks great! 👍

packages/common/src/directives/ng_class.ts Outdated Show resolved Hide resolved
@pkozlowski-opensource
Copy link
Member Author

De-flaked TGP

@pkozlowski-opensource pkozlowski-opensource added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed state: blocked action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 12, 2023
Copy link
Member

@alxhub alxhub left a comment

Choose a reason for hiding this comment

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

👏 this looks excellent.

The new algorithm is clear, easy to follow, and seems very efficient.

packages/common/src/directives/ng_class.ts Outdated Show resolved Hide resolved
packages/common/src/directives/ng_class.ts Outdated Show resolved Hide resolved
packages/common/src/directives/ng_class.ts Show resolved Hide resolved
@AndrewKushnir AndrewKushnir added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker action: global presubmit The PR is in need of a google3 global presubmit labels Jan 12, 2023
@pkozlowski-opensource pkozlowski-opensource removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 13, 2023
Remove unnecessary curly brackets in tests.
This commit represents rewrite of the NgClass directive to address
severe performance problem (excessive DOM mutation). The modified
algorithm removes all the known performance clifs and has number of
desirable properties:
- it is shorter and (arguably) easier to follow;
- drops usage of existing differs thus limiting dependencies on other
part of the code without increasing size of the directive;
- doesn't degrade any other performance metrics.

Fixes angular#25518
@pkozlowski-opensource
Copy link
Member Author

Caretaker note: the internal failing tests are not related and predate this PR. Also, I've got a passing, de-flaked TGP.

@pkozlowski-opensource pkozlowski-opensource added the action: merge The PR is ready for merge by the caretaker label Jan 13, 2023
@AndrewKushnir
Copy link
Contributor

This PR was merged into the repository by commit 5f21c6d.

AndrewKushnir pushed a commit that referenced this pull request Jan 13, 2023
This commit represents rewrite of the NgClass directive to address
severe performance problem (excessive DOM mutation). The modified
algorithm removes all the known performance clifs and has number of
desirable properties:
- it is shorter and (arguably) easier to follow;
- drops usage of existing differs thus limiting dependencies on other
part of the code without increasing size of the directive;
- doesn't degrade any other performance metrics.

Fixes #25518

PR Close #48433
AndrewKushnir pushed a commit that referenced this pull request Jan 13, 2023
Remove unnecessary curly brackets in tests.

PR Close #48433
AndrewKushnir pushed a commit that referenced this pull request Jan 13, 2023
This commit represents rewrite of the NgClass directive to address
severe performance problem (excessive DOM mutation). The modified
algorithm removes all the known performance clifs and has number of
desirable properties:
- it is shorter and (arguably) easier to follow;
- drops usage of existing differs thus limiting dependencies on other
part of the code without increasing size of the directive;
- doesn't degrade any other performance metrics.

Fixes #25518

PR Close #48433
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jan 19, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@angular/animations](https://github.com/angular/angular) | dependencies | patch | [`15.1.0` -> `15.1.1`](https://renovatebot.com/diffs/npm/@angular%2fanimations/15.1.0/15.1.1) |
| [@angular/common](https://github.com/angular/angular) | dependencies | patch | [`15.1.0` -> `15.1.1`](https://renovatebot.com/diffs/npm/@angular%2fcommon/15.1.0/15.1.1) |
| [@angular/compiler](https://github.com/angular/angular) | dependencies | patch | [`15.1.0` -> `15.1.1`](https://renovatebot.com/diffs/npm/@angular%2fcompiler/15.1.0/15.1.1) |
| [@angular/compiler-cli](https://github.com/angular/angular/tree/main/packages/compiler-cli) ([source](https://github.com/angular/angular)) | devDependencies | patch | [`15.1.0` -> `15.1.1`](https://renovatebot.com/diffs/npm/@angular%2fcompiler-cli/15.1.0/15.1.1) |
| [@angular/core](https://github.com/angular/angular) | dependencies | patch | [`15.1.0` -> `15.1.1`](https://renovatebot.com/diffs/npm/@angular%2fcore/15.1.0/15.1.1) |
| [@angular/forms](https://github.com/angular/angular) | dependencies | patch | [`15.1.0` -> `15.1.1`](https://renovatebot.com/diffs/npm/@angular%2fforms/15.1.0/15.1.1) |
| [@angular/platform-browser](https://github.com/angular/angular) | dependencies | patch | [`15.1.0` -> `15.1.1`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser/15.1.0/15.1.1) |
| [@angular/platform-browser-dynamic](https://github.com/angular/angular) | dependencies | patch | [`15.1.0` -> `15.1.1`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser-dynamic/15.1.0/15.1.1) |

---

### Release Notes

<details>
<summary>angular/angular</summary>

### [`v15.1.1`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#&#8203;1511-2023-01-18)

[Compare Source](angular/angular@15.1.0...15.1.1)

##### common

| Commit | Type | Description |
| -- | -- | -- |
| [68ce4f6ab4](angular/angular@68ce4f6) | fix | Update `Location` to get a normalized URL valid in case a represented URL starts with the substring equals `APP_BASE_HREF` ([#&#8203;48489](angular/angular#48489)) |
| [032b2bd689](angular/angular@032b2bd) | perf | avoid excessive DOM mutation in NgClass ([#&#8203;48433](angular/angular#48433)) |

##### core

| Commit | Type | Description |
| -- | -- | -- |
| [dd54f6bd96](angular/angular@dd54f6b) | fix | makeEnvironmentProviders should accept EnvironmentProviders ([#&#8203;48720](angular/angular#48720)) |

#### Special Thanks

Alan Agius, Alex Rickabaugh, Andrew Scott, Aristeidis Bampakos, Bob Watson, Jens, Konstantin Kharitonov, Kristiyan Kostadinov, Matthieu Riegler, Paul Gschwendtner, Pawel Kozlowski, Vladyslav Slipchenko, ced, dario-piotrowicz, mgechev and ノウラ

<!-- CHANGELOG SPLIT MARKER -->

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xMDUuMyIsInVwZGF0ZWRJblZlciI6IjM0LjEwNS41In0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1739
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
This commit represents rewrite of the NgClass directive to address
severe performance problem (excessive DOM mutation). The modified
algorithm removes all the known performance clifs and has number of
desirable properties:
- it is shorter and (arguably) easier to follow;
- drops usage of existing differs thus limiting dependencies on other
part of the code without increasing size of the directive;
- doesn't degrade any other performance metrics.

Fixes angular#25518

PR Close angular#48433
@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 Feb 13, 2023
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: common Issues related to APIs in the @angular/common package merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants