feat: add a flag in bootstrap to enable coalesce event to improve performance by JiaLiPassion · Pull Request #30533 · angular/angular

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

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

Suggested change

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

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

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

import global from core/src/util

got it, thanks!

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

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

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

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

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

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

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!

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

matsko added 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 17, 2019

JiaLiPassion force-pushed the test-coakescing-event branch 3 times, most recently from 8f6842d to 8f3b6ba Compare October 19, 2019 04:22

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

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

	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.

Conversation

JiaLiPassion commented May 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhevery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhevery commented May 31, 2019

Uh oh!

JiaLiPassion commented Jun 3, 2019

Uh oh!

mhevery commented Jun 4, 2019

Uh oh!

JiaLiPassion commented Jun 7, 2019

Uh oh!

JiaLiPassion commented Oct 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhevery commented Oct 28, 2019

Uh oh!

mhevery commented Nov 5, 2019

Uh oh!

AndrewKushnir commented Nov 5, 2019

Uh oh!

atscott commented Nov 5, 2019

Uh oh!

naveedahmed1 commented Nov 7, 2019

Uh oh!

trotyl commented Nov 7, 2019

Uh oh!

JiaLiPassion commented Nov 7, 2019

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

JiaLiPassion commented May 17, 2019 •

edited

Loading