Skip to content

Commit

Permalink
feat(core): add shouldCoalesceRunChangeDetection option to coalesce c…
Browse files Browse the repository at this point in the history
…hange detections in the same event loop. (#39422)

Close #39348

Now `NgZone` has an option `shouldCoalesceEventChangeDetection` to coalesce
multiple event handler's change detections to one async change detection.

And there are some cases other than `event handler` have the same issues.
In #39348, the case like this.

```
// This code results in one change detection occurring per
// ngZone.run() call. This is entirely feasible, and can be a serious
// performance issue.
for (let i = 0; i < 100; i++) {
  this.ngZone.run(() => {
    // do something
  });
}
```

So such kind of case will trigger multiple change detections.
And now with Ivy, we have a new `markDirty()` API will schedule
a requestAnimationFrame to trigger change detection and also coalesce
the change detections in the same event loop, `markDirty()` API doesn't
only take care `event handler` but also all other cases `sync/macroTask/..`

So this PR add a new option to coalesce change detections for all cases.

test(core): add test case for shouldCoalesceEventChangeDetection option

Add new test cases for current `shouldCoalesceEventChangeDetection` in `ng_zone.spec`, since
currently we only have integration test for this one.

PR Close #39422
  • Loading branch information
JiaLiPassion authored and atscott committed Nov 16, 2020
1 parent d68cac6 commit 5e92d64
Show file tree
Hide file tree
Showing 6 changed files with 451 additions and 59 deletions.
3 changes: 2 additions & 1 deletion goldens/public-api/core/core.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -619,9 +619,10 @@ export declare class NgZone {
readonly onMicrotaskEmpty: EventEmitter<any>;
readonly onStable: EventEmitter<any>;
readonly onUnstable: EventEmitter<any>;
constructor({ enableLongStackTrace, shouldCoalesceEventChangeDetection }: {
constructor({ enableLongStackTrace, shouldCoalesceEventChangeDetection, shouldCoalesceRunChangeDetection }: {
enableLongStackTrace?: boolean | undefined;
shouldCoalesceEventChangeDetection?: boolean | undefined;
shouldCoalesceRunChangeDetection?: boolean | undefined;
});
run<T>(fn: (...args: any[]) => T, applyThis?: any, applyArgs?: any[]): T;
runGuarded<T>(fn: (...args: any[]) => T, applyThis?: any, applyArgs?: any[]): T;
Expand Down
14 changes: 7 additions & 7 deletions goldens/size-tracking/integration-payloads.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"master": {
"uncompressed": {
"runtime-es2015": 1485,
"main-es2015": 140899,
"main-es2015": 141516,
"polyfills-es2015": 36964
}
}
Expand Down Expand Up @@ -39,7 +39,7 @@
"master": {
"uncompressed": {
"runtime-es2015": 2285,
"main-es2015": 241875,
"main-es2015": 242417,
"polyfills-es2015": 36709,
"5-es2015": 745
}
Expand All @@ -48,10 +48,10 @@
"cli-hello-world-lazy-rollup": {
"master": {
"uncompressed": {
"runtime-es2015": 2285,
"main-es2015": 218340,
"polyfills-es2015": 36709,
"5-es2015": 777
"runtime-es2015": 2289,
"main-es2015": 218507,
"polyfills-es2015": 36723,
"5-es2015": 781
}
}
},
Expand All @@ -66,4 +66,4 @@
}
}
}
}
}
34 changes: 29 additions & 5 deletions packages/core/src/application_ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,25 @@ export interface BootstrapOptions {
* the change detection will only be triggered once.
*/
ngZoneEventCoalescing?: boolean;

/**
* Optionally specify if `NgZone#run()` method invocations should be coalesced
* into a single change detection.
*
* Consider the following case.
*
* for (let i = 0; i < 10; i ++) {
* ngZone.run(() => {
* // do something
* });
* }
*
* This case triggers the change detection multiple times.
* With ngZoneRunCoalescing options, all change detections in an event loop trigger only once.
* In addition, the change detection executes in requestAnimation.
*
*/
ngZoneRunCoalescing?: boolean;
}

/**
Expand Down Expand Up @@ -316,10 +335,13 @@ export class PlatformRef {
// pass that as parent to the NgModuleFactory.
const ngZoneOption = options ? options.ngZone : undefined;
const ngZoneEventCoalescing = (options && options.ngZoneEventCoalescing) || false;
const ngZone = getNgZone(ngZoneOption, ngZoneEventCoalescing);
const ngZoneRunCoalescing = (options && options.ngZoneRunCoalescing) || false;
const ngZone = getNgZone(ngZoneOption, {ngZoneEventCoalescing, ngZoneRunCoalescing});
const providers: StaticProvider[] = [{provide: NgZone, useValue: ngZone}];
// Attention: Don't use ApplicationRef.run here,
// as we want to be sure that all possible constructor calls are inside `ngZone.run`!
// Note: Create ngZoneInjector within ngZone.run so that all of the instantiated services are
// created within the Angular zone
// Do not try to replace ngZone.run with ApplicationRef#run because ApplicationRef would then be
// created outside of the Angular zone.
return ngZone.run(() => {
const ngZoneInjector = Injector.create(
{providers: providers, parent: this.injector, name: moduleFactory.moduleType.name});
Expand Down Expand Up @@ -426,15 +448,17 @@ export class PlatformRef {
}

function getNgZone(
ngZoneOption: NgZone|'zone.js'|'noop'|undefined, ngZoneEventCoalescing: boolean): NgZone {
ngZoneOption: NgZone|'zone.js'|'noop'|undefined,
extra?: {ngZoneEventCoalescing: boolean, ngZoneRunCoalescing: boolean}): NgZone {
let ngZone: NgZone;

if (ngZoneOption === 'noop') {
ngZone = new NoopNgZone();
} else {
ngZone = (ngZoneOption === 'zone.js' ? undefined : ngZoneOption) || new NgZone({
enableLongStackTrace: isDevMode(),
shouldCoalesceEventChangeDetection: ngZoneEventCoalescing
shouldCoalesceEventChangeDetection: !!extra?.ngZoneEventCoalescing,
shouldCoalesceRunChangeDetection: !!extra?.ngZoneRunCoalescing
});
}
return ngZone;
Expand Down
72 changes: 59 additions & 13 deletions packages/core/src/zone/ng_zone.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,11 @@ export class NgZone {
readonly onError: EventEmitter<any> = new EventEmitter(false);


constructor({enableLongStackTrace = false, shouldCoalesceEventChangeDetection = false}) {
constructor({
enableLongStackTrace = false,
shouldCoalesceEventChangeDetection = false,
shouldCoalesceRunChangeDetection = false
}) {
if (typeof Zone == 'undefined') {
throw new Error(`In this configuration Angular requires Zone.js`);
}
Expand All @@ -137,8 +141,11 @@ export class NgZone {
if (enableLongStackTrace && (Zone as any)['longStackTraceZoneSpec']) {
self._inner = self._inner.fork((Zone as any)['longStackTraceZoneSpec']);
}

self.shouldCoalesceEventChangeDetection = shouldCoalesceEventChangeDetection;
// if shouldCoalesceRunChangeDetection is true, all tasks including event tasks will be
// coalesced, so shouldCoalesceEventChangeDetection option is not necessary and can be skipped.
self.shouldCoalesceEventChangeDetection =
!shouldCoalesceRunChangeDetection && shouldCoalesceEventChangeDetection;
self.shouldCoalesceRunChangeDetection = shouldCoalesceRunChangeDetection;
self.lastRequestAnimationFrameId = -1;
self.nativeRequestAnimationFrame = getNativeRequestAnimationFrame().nativeRequestAnimationFrame;
forkInnerZoneWithAngularBehavior(self);
Expand Down Expand Up @@ -237,11 +244,49 @@ interface NgZonePrivate extends NgZone {
hasPendingMicrotasks: boolean;
lastRequestAnimationFrameId: number;
isStable: boolean;
/**
* Optionally specify coalescing event change detections or not.
* Consider the following case.
*
* <div (click)="doSomething()">
* <button (click)="doSomethingElse()"></button>
* </div>
*
* When button is clicked, because of the event bubbling, both
* event handlers will be called and 2 change detections will be
* triggered. We can coalesce such kind of events to trigger
* change detection only once.
*
* By default, this option will be false. So the events will not be
* coalesced and the change detection will be triggered multiple times.
* And if this option be set to true, the change detection will be
* triggered async by scheduling it in an animation frame. So in the case above,
* the change detection will only be trigged once.
*/
shouldCoalesceEventChangeDetection: boolean;
/**
* Optionally specify if `NgZone#run()` method invocations should be coalesced
* into a single change detection.
*
* Consider the following case.
*
* for (let i = 0; i < 10; i ++) {
* ngZone.run(() => {
* // do something
* });
* }
*
* This case triggers the change detection multiple times.
* With ngZoneRunCoalescing options, all change detections in an event loops trigger only once.
* In addition, the change detection executes in requestAnimation.
*
*/
shouldCoalesceRunChangeDetection: boolean;

nativeRequestAnimationFrame: (callback: FrameRequestCallback) => number;

// Cache of "fake" top eventTask. This is done so that we don't need to schedule a new task every
// time we want to run a `checkStable`.
// Cache a "fake" top eventTask so you don't need to schedule a new task every
// time you run a `checkStable`.
fakeTopEventTask: Task;
}

Expand Down Expand Up @@ -293,34 +338,34 @@ function forkInnerZoneWithAngularBehavior(zone: NgZonePrivate) {
const delayChangeDetectionForEventsDelegate = () => {
delayChangeDetectionForEvents(zone);
};
const maybeDelayChangeDetection = !!zone.shouldCoalesceEventChangeDetection &&
zone.nativeRequestAnimationFrame && delayChangeDetectionForEventsDelegate;
zone._inner = zone._inner.fork({
name: 'angular',
properties:
<any>{'isAngularZone': true, 'maybeDelayChangeDetection': maybeDelayChangeDetection},
properties: <any>{'isAngularZone': true},
onInvokeTask:
(delegate: ZoneDelegate, current: Zone, target: Zone, task: Task, applyThis: any,
applyArgs: any): any => {
try {
onEnter(zone);
return delegate.invokeTask(target, task, applyThis, applyArgs);
} finally {
if (maybeDelayChangeDetection && task.type === 'eventTask') {
maybeDelayChangeDetection();
if ((zone.shouldCoalesceEventChangeDetection && task.type === 'eventTask') ||
zone.shouldCoalesceRunChangeDetection) {
delayChangeDetectionForEventsDelegate();
}
onLeave(zone);
}
},


onInvoke:
(delegate: ZoneDelegate, current: Zone, target: Zone, callback: Function, applyThis: any,
applyArgs?: any[], source?: string): any => {
try {
onEnter(zone);
return delegate.invoke(target, callback, applyThis, applyArgs, source);
} finally {
if (zone.shouldCoalesceRunChangeDetection) {
delayChangeDetectionForEventsDelegate();
}
onLeave(zone);
}
},
Expand Down Expand Up @@ -351,7 +396,8 @@ function forkInnerZoneWithAngularBehavior(zone: NgZonePrivate) {

function updateMicroTaskStatus(zone: NgZonePrivate) {
if (zone._hasPendingMicrotasks ||
(zone.shouldCoalesceEventChangeDetection && zone.lastRequestAnimationFrameId !== -1)) {
((zone.shouldCoalesceEventChangeDetection || zone.shouldCoalesceRunChangeDetection) &&
zone.lastRequestAnimationFrameId !== -1)) {
zone.hasPendingMicrotasks = true;
} else {
zone.hasPendingMicrotasks = false;
Expand Down

0 comments on commit 5e92d64

Please sign in to comment.