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

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

Closed

Conversation

@JiaLiPassion
Copy link
Contributor

JiaLiPassion commented May 17, 2019

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.

@JiaLiPassion JiaLiPassion requested a review from angular/fw-core as a code owner May 17, 2019
@googlebot googlebot added the cla: yes label May 17, 2019
@ngbot ngbot bot added this to the needsTriage milestone May 17, 2019
@JiaLiPassion JiaLiPassion force-pushed the JiaLiPassion:test-coakescing-event branch from b87b7a3 to b50e1ae May 18, 2019
Copy link
Member

mhevery left a comment

Minor comments.

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

This comment has been minimized.

Copy link
@mhevery

mhevery May 31, 2019

Member

I think this is easier to read.

Suggested change
const ngZoneEventCoalescing = options ? options.ngZoneEventCoalescing : false;
const ngZoneEventCoalescing = (options && options.ngZoneEventCoalescing) || false;
@@ -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 {

This comment has been minimized.

Copy link
@mhevery

mhevery May 31, 2019

Member
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 Outdated Show resolved Hide resolved
packages/core/src/zone/ng_zone.ts Outdated Show resolved Hide resolved
packages/core/src/zone/ng_zone.ts Outdated Show resolved Hide resolved
function forkInnerZoneWithAngularBehavior(zone: NgZonePrivate) {
declare const global: any;

const _global: any = typeof window !== 'undefined' ?

This comment has been minimized.

Copy link
@mhevery

mhevery May 31, 2019

Member

import global from core/src/util

This comment has been minimized.

Copy link
@JiaLiPassion

JiaLiPassion May 31, 2019

Author Contributor

got it, thanks!

packages/core/src/zone/ng_zone.ts Outdated Show resolved Hide resolved
// so we need to cancel that and schedule a new one, or maybe we can just
// skip.
// option1 : cancel and reschedule
// option2: just return

This comment has been minimized.

Copy link
@mhevery

mhevery May 31, 2019

Member

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

This comment has been minimized.

Copy link
@JiaLiPassion

JiaLiPassion May 31, 2019

Author Contributor

Got it!

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

ngZoneEventCoalescing?: boolean;

This comment has been minimized.

Copy link
@mhevery

mhevery May 31, 2019

Member

Needs public API docs here.

This comment has been minimized.

Copy link
@mhevery

mhevery Sep 17, 2019

Member

can you add docs explaining what this does?

@mhevery

This comment has been minimized.

Copy link
Member

mhevery commented May 31, 2019

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

@JiaLiPassion JiaLiPassion requested a review from angular/fw-public-api as a code owner May 31, 2019
@JiaLiPassion

This comment has been minimized.

Copy link
Contributor Author

JiaLiPassion commented Jun 3, 2019

@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

This comment has been minimized.

@JiaLiPassion JiaLiPassion force-pushed the JiaLiPassion:test-coakescing-event branch from 36e5659 to 38a3b0b Jun 6, 2019
@JiaLiPassion JiaLiPassion requested a review from angular/fw-integration as a code owner Jun 6, 2019
@JiaLiPassion JiaLiPassion changed the title WIP: test coalesce event feat: test coalesce event Jun 6, 2019
@JiaLiPassion 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

This comment has been minimized.

Copy link
Contributor Author

JiaLiPassion commented Jun 7, 2019

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

@JiaLiPassion JiaLiPassion force-pushed the JiaLiPassion:test-coakescing-event branch 2 times, most recently from e45f1fc to b0ec7b4 Jul 24, 2019
@JiaLiPassion JiaLiPassion requested a review from angular/docs-infra as a code owner Jul 25, 2019
@JiaLiPassion JiaLiPassion force-pushed the JiaLiPassion:test-coakescing-event branch from b0ec7b4 to 1bc95b4 Jul 26, 2019
@JiaLiPassion JiaLiPassion requested a review from angular/fw-dev-infra as a code owner Jul 26, 2019
@JiaLiPassion JiaLiPassion force-pushed the JiaLiPassion:test-coakescing-event branch 2 times, most recently from 665c1ae to 0e681c1 Jul 26, 2019
@JiaLiPassion JiaLiPassion force-pushed the JiaLiPassion:test-coakescing-event branch 5 times, most recently from df3dd8c to 5911811 Aug 30, 2019
@JiaLiPassion JiaLiPassion requested a review from mhevery Aug 30, 2019
@JiaLiPassion JiaLiPassion force-pushed the JiaLiPassion:test-coakescing-event branch 2 times, most recently from 8f6842d to 8f3b6ba Oct 19, 2019
@JiaLiPassion

This comment has been minimized.

Copy link
Contributor Author

JiaLiPassion commented Oct 19, 2019

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

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

This comment has been minimized.

Copy link
@devversion

devversion Oct 19, 2019

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

This comment has been minimized.

Copy link
@JiaLiPassion

JiaLiPassion Oct 19, 2019

Author Contributor

thanks for the review, updated.

};
}

var nativeRequestAnimationFrame = getNativeRequestAnimationFrame().nativeRequestAnimationFrame;

This comment has been minimized.

Copy link
@devversion

devversion Oct 19, 2019

Member

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

This comment has been minimized.

Copy link
@JiaLiPassion

JiaLiPassion Oct 19, 2019

Author Contributor

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

@JiaLiPassion JiaLiPassion force-pushed the JiaLiPassion:test-coakescing-event branch 2 times, most recently from 0743288 to be89686 Oct 19, 2019
…on to improve performance
@JiaLiPassion JiaLiPassion force-pushed the JiaLiPassion:test-coakescing-event branch from be89686 to 90ca4cb Oct 19, 2019
@JiaLiPassion JiaLiPassion requested a review from devversion Oct 19, 2019
@kara kara requested a review from IgorMinar Oct 22, 2019
AndrusGerman added a commit to AndrusGerman/angular that referenced this pull request Oct 22, 2019
AndrusGerman added a commit to AndrusGerman/angular that referenced this pull request Oct 22, 2019
@JiaLiPassion JiaLiPassion requested a review from mhevery Oct 26, 2019
@mhevery

This comment has been minimized.

Copy link
Member

mhevery commented Oct 28, 2019

@mhevery

This comment has been minimized.

Copy link
Member

mhevery commented Nov 5, 2019

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

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Nov 5, 2019

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

@atscott atscott closed this in 44623a1 Nov 5, 2019
@atscott

This comment has been minimized.

Copy link
Contributor

atscott commented Nov 5, 2019

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

@naveedahmed1

This comment has been minimized.

Copy link

naveedahmed1 commented Nov 7, 2019

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

@trotyl

This comment has been minimized.

Copy link
Contributor

trotyl commented Nov 7, 2019

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

This comment has been minimized.

Copy link
Contributor Author

JiaLiPassion commented Nov 7, 2019

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

mohaxspb added a commit to mohaxspb/angular that referenced this pull request Nov 7, 2019
mohaxspb added a commit to mohaxspb/angular that referenced this pull request Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.