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

Hooks refactor #32131

Conversation

@pkozlowski-opensource
Copy link
Member

commented Aug 14, 2019

This PR splits hooks processing functions into:

  • init-specific (slower but less common);
  • check-specific (faster and more common).

On a local machine this change speeds up the noop_change_detection micro-benchmark from ~1.7s down to ~0.8s (x2 improvement).

Check individual commit messages for more details.

@googlebot googlebot added the cla: yes label Aug 14, 2019

@pkozlowski-opensource pkozlowski-opensource force-pushed the pkozlowski-opensource:hooks_refactor branch 5 times, most recently from ee56951 to 83529b8 Aug 14, 2019

@ngbot ngbot bot modified the milestone: needsTriage Aug 19, 2019

@pkozlowski-opensource pkozlowski-opensource marked this pull request as ready for review Aug 19, 2019

@pkozlowski-opensource pkozlowski-opensource requested review from angular/fw-core as code owners Aug 19, 2019

@mhevery
Copy link
Member

left a comment

LGTM, but I have to say that I am not sure how I feel about so much code duplication. I wonder if we could somehow unify them. Happy to brainstorm with you on it.

packages/core/src/render3/hooks.ts Show resolved Hide resolved
packages/core/src/render3/hooks.ts Outdated Show resolved Hide resolved
packages/core/src/render3/hooks.ts Outdated Show resolved Hide resolved
packages/core/src/render3/hooks.ts Outdated Show resolved Hide resolved

@pkozlowski-opensource pkozlowski-opensource force-pushed the pkozlowski-opensource:hooks_refactor branch from 83529b8 to f95b75b Aug 21, 2019

perf(ivy): split hooks processing into init and check phases
Angular hooks come after 2 flavours:
- init hooks (OnInit, AfterContentInit, AfterViewInit);
- check hooks (OnChanges, DoChanges, AfterContentChecked, AfterViewChecked).

We need to do more processing for init hooks to ensure that those hooks
are run once and only once for a given directive (even in case of errors).
As soon as all init hooks execute to completion we are only left with the
checks to execute.

It turns out that keeping track of the remaining init hooks to execute is
rather expensive (multiple LView flags reads, writes and checks). But we can
observe that non of this tracking is needed as soon as all init hooks are
completed.

This PR takes advantage of the above observations and splits hooks processing
functions into:
- init-specific (slower but less common);
- check-specific (faster and more common).

NOTE: there is code duplication in this PR and it is left like this intentinally:
hand-inlining this perf-critical code makes the view refresh process substentially
faster.

@pkozlowski-opensource pkozlowski-opensource force-pushed the pkozlowski-opensource:hooks_refactor branch from f95b75b to 0586146 Aug 21, 2019

@kara
kara approved these changes Aug 21, 2019
Copy link
Contributor

left a comment

LGTM

@kara

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

AndrewKushnir added a commit that referenced this pull request Aug 21, 2019
perf(ivy): split hooks processing into init and check phases (#32131)
Angular hooks come after 2 flavours:
- init hooks (OnInit, AfterContentInit, AfterViewInit);
- check hooks (OnChanges, DoChanges, AfterContentChecked, AfterViewChecked).

We need to do more processing for init hooks to ensure that those hooks
are run once and only once for a given directive (even in case of errors).
As soon as all init hooks execute to completion we are only left with the
checks to execute.

It turns out that keeping track of the remaining init hooks to execute is
rather expensive (multiple LView flags reads, writes and checks). But we can
observe that non of this tracking is needed as soon as all init hooks are
completed.

This PR takes advantage of the above observations and splits hooks processing
functions into:
- init-specific (slower but less common);
- check-specific (faster and more common).

NOTE: there is code duplication in this PR and it is left like this intentinally:
hand-inlining this perf-critical code makes the view refresh process substentially
faster.

PR Close #32131
ngdevelop-tech added a commit to ngdevelop-tech/angular that referenced this pull request Aug 27, 2019
ngdevelop-tech added a commit to ngdevelop-tech/angular that referenced this pull request Aug 27, 2019
perf(ivy): split hooks processing into init and check phases (angular…
…#32131)

Angular hooks come after 2 flavours:
- init hooks (OnInit, AfterContentInit, AfterViewInit);
- check hooks (OnChanges, DoChanges, AfterContentChecked, AfterViewChecked).

We need to do more processing for init hooks to ensure that those hooks
are run once and only once for a given directive (even in case of errors).
As soon as all init hooks execute to completion we are only left with the
checks to execute.

It turns out that keeping track of the remaining init hooks to execute is
rather expensive (multiple LView flags reads, writes and checks). But we can
observe that non of this tracking is needed as soon as all init hooks are
completed.

This PR takes advantage of the above observations and splits hooks processing
functions into:
- init-specific (slower but less common);
- check-specific (faster and more common).

NOTE: there is code duplication in this PR and it is left like this intentinally:
hand-inlining this perf-critical code makes the view refresh process substentially
faster.

PR Close angular#32131
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
perf(ivy): split hooks processing into init and check phases (angular…
…#32131)

Angular hooks come after 2 flavours:
- init hooks (OnInit, AfterContentInit, AfterViewInit);
- check hooks (OnChanges, DoChanges, AfterContentChecked, AfterViewChecked).

We need to do more processing for init hooks to ensure that those hooks
are run once and only once for a given directive (even in case of errors).
As soon as all init hooks execute to completion we are only left with the
checks to execute.

It turns out that keeping track of the remaining init hooks to execute is
rather expensive (multiple LView flags reads, writes and checks). But we can
observe that non of this tracking is needed as soon as all init hooks are
completed.

This PR takes advantage of the above observations and splits hooks processing
functions into:
- init-specific (slower but less common);
- check-specific (faster and more common).

NOTE: there is code duplication in this PR and it is left like this intentinally:
hand-inlining this perf-critical code makes the view refresh process substentially
faster.

PR Close angular#32131
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.