feat: add a flag in bootstrap to enable coalesce event to improve performance #30533

JiaLiPassion · 2019-05-17T11:51:08Z

add a new flag in BootstrapOptions to enable coalesce event to improve performance.

/**
 * Provides additional options to the bootstraping process.
 *
 *
 */
export interface BootstrapOptions {
  /**
   * Optionally specify which `NgZone` should be used.
   *
   * - Provide your own `NgZone` instance.
   * - `zone.js` - Use default `NgZone` which requires `Zone.js`.
   * - `noop` - Use `NoopNgZone` which does nothing.
   */
  ngZone?: NgZone|'zone.js'|'noop';
  
  // START<<<<<<<<<
  /**
   * Optionally specify if `NgZone` should be configure to coalesce
   * events.
   */
  ngZoneEventCoalescing?: true|false;
  // END<<<<<<<<<<<
}

The use case is something like this,

<div (click)="doSomethingElse()">
  <button (click)="doSomething()">click me</button>
</div>

It turns out that as the click event bubbles from the towards the root document it triggers change detection on each listener. This example shows the phenomena in interactive form. Notice that clicking on the will run the change detection twice. Once for the and once for the

.

So with this PR, we have this new option to let ngZone only run Change Detection once in this case.
By default, this new ngZoneEventCoalescing will be false, so nothing changed. If this option is being set to true, then only one change detection will be triggered async.

mhevery

Minor comments.

mhevery · 2019-05-31T02:58:18Z

packages/core/src/application_ref.ts

@@ -249,7 +251,8 @@ export class PlatformRef {
    // So we create a mini parent injector that just contains the new NgZone and
    // pass that as parent to the NgModuleFactory.
    const ngZoneOption = options ? options.ngZone : undefined;
-    const ngZone = getNgZone(ngZoneOption);
+    const ngZoneEventCoalescing = options ? options.ngZoneEventCoalescing : false;


I think this is easier to read.

Suggested change

const ngZoneEventCoalescing = options ? options.ngZoneEventCoalescing : false;

const ngZoneEventCoalescing = (options && options.ngZoneEventCoalescing) || false;

mhevery · 2019-05-31T02:59:53Z

packages/core/src/application_ref.ts

@@ -340,14 +343,17 @@ export class PlatformRef {
  get destroyed() { return this._destroyed; }
 }

-function getNgZone(ngZoneOption?: NgZone | 'zone.js' | 'noop'): NgZone {
+function getNgZone(
+    ngZoneOption?: NgZone | 'zone.js' | 'noop', ngZoneEventCoalescing?: boolean): NgZone {


Suggested change

ngZoneOption?: NgZone | 'zone.js' | 'noop', ngZoneEventCoalescing?: boolean): NgZone {

ngZoneOption: NgZone | 'zone.js' | 'noop' | undefiend, ngZoneEventCoalescing: boolean): NgZone {

packages/core/src/zone/ng_zone.ts

mhevery · 2019-05-31T03:09:18Z

packages/core/src/zone/ng_zone.ts

-function forkInnerZoneWithAngularBehavior(zone: NgZonePrivate) {
+declare const global: any;
+
+const _global: any = typeof window !== 'undefined' ?


import global from core/src/util

got it, thanks!

packages/core/src/zone/ng_zone.ts

mhevery · 2019-05-31T03:10:26Z

packages/core/src/zone/ng_zone.ts

+    // so we need to cancel that and schedule a new one, or maybe we can just
+    // skip.
+    // option1 : cancel and reschedule
+    // option2: just return


Just return, I don't think there is a value in canceling it.

mhevery · 2019-05-31T03:14:14Z

packages/core/src/application_ref.ts

@@ -199,6 +199,8 @@ export interface BootstrapOptions {
   * - `noop` - Use `NoopNgZone` which does nothing.
   */
  ngZone?: NgZone|'zone.js'|'noop';
+
+  ngZoneEventCoalescing?: boolean;


Needs public API docs here.

can you add docs explaining what this does?

mhevery · 2019-05-31T03:16:50Z

Need to run bazel run //tools/public_api_guard:core_api.accept

JiaLiPassion · 2019-06-03T16:09:59Z

@mhevery, I fixed several issues, now all tests passed, (expect the case of integration file-size-limit), I will continue to add test cases, could you run this version in g3? Thanks!

mhevery · 2019-06-04T20:30:38Z

http://test/OCL:251502926:BASE:251502950:1559679912312:8b435b5c

JiaLiPassion · 2019-06-07T03:08:18Z

Need wait angular/zone.js#1239 to be merged. So the integration test can pass.

JiaLiPassion · 2019-10-19T04:38:34Z

@matsko, I have updated the PR and now the API will not have any changes, and g3 should be ok, please review, thank you!

devversion · 2019-10-19T11:03:39Z

packages/core/src/application_ref.ts

+   * change detection only once.
+   *
+   * By default, this option will be false. So the events will not be
+   * colesced and the change detection will be triggered multiple times.


Suggested change

* colesced and the change detection will be triggered multiple times.

* coalesced and the change detection will be triggered multiple times.

thanks for the review, updated.

devversion · 2019-10-19T11:14:27Z

integration/side-effects/snapshots/core/esm5.js

+    };
+}
+
+var nativeRequestAnimationFrame = getNativeRequestAnimationFrame().nativeRequestAnimationFrame;


I just stumbled over this PR. Out of curiosity: is it intentional that we introduce a new side-effect global call here?

you are right, I should not add a side-effect here, I just updated the PR to move the call inside ngZone initialize call.

…on to improve performance

…on to improve performance (angular#30533) PR Close angular#30533

… detection to improve performance (angular#30533) (angular#33230) This reverts commit 21c1e14. PR Close angular#33230

mhevery · 2019-10-28T16:49:50Z

presubmit

mhevery · 2019-11-05T00:07:04Z

MERGE_ASSISTANCE: Please merge by itself as this is high risk.

AndrewKushnir · 2019-11-05T03:07:27Z

Since this change is a high risk (as Misko mentioned), I've started a Global VE presubmit as well (and a Blueprint one).

atscott · 2019-11-05T19:01:07Z

This was merged to master only. @mhevery @JiaLiPassion is that good or did you also need this in the patch branch?

naveedahmed1 · 2019-11-07T00:31:12Z

Should this provide any benefit on server, I mean in case of SSR/Universal?

trotyl · 2019-11-07T05:40:28Z

Should this provide any benefit on server, I mean in case of SSR/Universal?

There is little chance for DOM events to be triggered on server, unless you manually dispatch them.

JiaLiPassion · 2019-11-07T13:43:10Z

@atscott, I am not sure about whether this need to be merged to patch branch, @mhevery , please confirm, thanks!

…on to improve performance (angular#30533) PR Close angular#30533

angular-automatic-lock-bot · 2019-12-08T16:03:56Z

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.}

JiaLiPassion requested a review from a team as a code owner May 17, 2019 11:51

googlebot added the cla: yes label May 17, 2019

jasonaden added the area: core Issues related to the framework runtime label May 17, 2019

ngbot bot added this to the needsTriage milestone May 17, 2019

JiaLiPassion force-pushed the test-coakescing-event branch from b87b7a3 to b50e1ae Compare May 18, 2019 03:55

mhevery suggested changes May 31, 2019

View reviewed changes

JiaLiPassion requested a review from a team as a code owner May 31, 2019 15:30

JiaLiPassion force-pushed the test-coakescing-event branch from 36e5659 to 38a3b0b Compare June 6, 2019 16:43

JiaLiPassion requested a review from a team as a code owner June 6, 2019 16:43

JiaLiPassion changed the title ~~WIP: test coalesce event~~ feat: test coalesce event Jun 6, 2019

JiaLiPassion changed the title ~~feat: test coalesce event~~ feat: add a flag in bootstrap to enable coalesce event to improve performance Jun 6, 2019

JiaLiPassion mentioned this pull request Jun 7, 2019

build: add __load_patch and __symbol__ in zone_extern for closure angular/zone.js#1239

Closed

JiaLiPassion mentioned this pull request Jun 29, 2019

fix(zone.js): __load_patch and __symbol__ should be in zone_extern for closure compiler #31350

Closed

14 tasks

JiaLiPassion force-pushed the test-coakescing-event branch 2 times, most recently from e45f1fc to b0ec7b4 Compare July 25, 2019 04:54

JiaLiPassion requested a review from a team as a code owner July 25, 2019 04:54

JiaLiPassion force-pushed the test-coakescing-event branch from b0ec7b4 to 1bc95b4 Compare July 26, 2019 20:33

JiaLiPassion requested a review from a team as a code owner July 26, 2019 20:33

JiaLiPassion force-pushed the test-coakescing-event branch 2 times, most recently from 665c1ae to 0e681c1 Compare July 26, 2019 21:32

JiaLiPassion force-pushed the test-coakescing-event branch 5 times, most recently from df3dd8c to 5911811 Compare August 30, 2019 06:23

JiaLiPassion requested a review from mhevery August 30, 2019 15:06

JiaLiPassion 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 Oct 19, 2019

devversion reviewed Oct 19, 2019

View reviewed changes

JiaLiPassion force-pushed the test-coakescing-event branch 2 times, most recently from 0743288 to be89686 Compare October 19, 2019 13:50

feat: add a flag in bootstrap to enable coalesce event change detecti…

90ca4cb

…on to improve performance

JiaLiPassion force-pushed the test-coakescing-event branch from be89686 to 90ca4cb Compare October 19, 2019 14:09

JiaLiPassion requested a review from devversion October 19, 2019 14:16

matsko added the action: merge The PR is ready for merge by the caretaker label Oct 21, 2019

kara added the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 22, 2019

kara requested a review from IgorMinar October 22, 2019 19:17

AndrusGerman pushed a commit to AndrusGerman/angular that referenced this pull request Oct 22, 2019

feat: add a flag in bootstrap to enable coalesce event change detecti…

7efeb16

…on to improve performance (angular#30533) PR Close angular#30533

JiaLiPassion requested a review from mhevery October 26, 2019 02:11

mhevery removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 29, 2019

mhevery approved these changes Oct 29, 2019

View reviewed changes

AndrewKushnir added the risk: high label Oct 29, 2019

mhevery added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Nov 5, 2019

atscott closed this in 44623a1 Nov 5, 2019

mohaxspb pushed a commit to mohaxspb/angular that referenced this pull request Nov 7, 2019

feat: add a flag in bootstrap to enable coalesce event change detecti…

fae0d7a

…on to improve performance (angular#30533) PR Close angular#30533

mohaxspb pushed a commit to mohaxspb/angular that referenced this pull request Nov 7, 2019

feat: add a flag in bootstrap to enable coalesce event change detecti…

ba97929

…on to improve performance (angular#30533) PR Close angular#30533

angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add a flag in bootstrap to enable coalesce event to improve performance #30533

feat: add a flag in bootstrap to enable coalesce event to improve performance #30533

JiaLiPassion commented May 17, 2019 •

edited

mhevery left a comment

mhevery May 31, 2019

mhevery May 31, 2019

mhevery May 31, 2019

JiaLiPassion May 31, 2019

mhevery May 31, 2019

JiaLiPassion May 31, 2019

mhevery May 31, 2019

mhevery Sep 17, 2019

mhevery commented May 31, 2019

JiaLiPassion commented Jun 3, 2019

mhevery commented Jun 4, 2019

JiaLiPassion commented Jun 7, 2019

JiaLiPassion commented Oct 19, 2019

devversion Oct 19, 2019

JiaLiPassion Oct 19, 2019

devversion Oct 19, 2019

JiaLiPassion Oct 19, 2019

mhevery commented Oct 28, 2019

mhevery commented Nov 5, 2019

AndrewKushnir commented Nov 5, 2019

atscott commented Nov 5, 2019

naveedahmed1 commented Nov 7, 2019

trotyl commented Nov 7, 2019

JiaLiPassion commented Nov 7, 2019

angular-automatic-lock-bot bot commented Dec 8, 2019

	const ngZoneEventCoalescing = options ? options.ngZoneEventCoalescing : false;
	const ngZoneEventCoalescing = (options && options.ngZoneEventCoalescing) \|\| false;

	ngZoneOption?: NgZone \| 'zone.js' \| 'noop', ngZoneEventCoalescing?: boolean): NgZone {
	ngZoneOption: NgZone \| 'zone.js' \| 'noop' \| undefiend, ngZoneEventCoalescing: boolean): NgZone {

	* colesced and the change detection will be triggered multiple times.
	* coalesced and the change detection will be triggered multiple times.

feat: add a flag in bootstrap to enable coalesce event to improve performance #30533

feat: add a flag in bootstrap to enable coalesce event to improve performance #30533

Conversation

JiaLiPassion commented May 17, 2019 • edited

mhevery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhevery commented May 31, 2019

JiaLiPassion commented Jun 3, 2019

mhevery commented Jun 4, 2019

JiaLiPassion commented Jun 7, 2019

JiaLiPassion commented Oct 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhevery commented Oct 28, 2019

mhevery commented Nov 5, 2019

AndrewKushnir commented Nov 5, 2019

atscott commented Nov 5, 2019

naveedahmed1 commented Nov 7, 2019

trotyl commented Nov 7, 2019

JiaLiPassion commented Nov 7, 2019

angular-automatic-lock-bot bot commented Dec 8, 2019

JiaLiPassion commented May 17, 2019 •

edited