Skip to content

Commit

Permalink
fix(core): ensure change detection runs in a reasonable timeframe wit…
Browse files Browse the repository at this point in the history
…h zone coalescing (#54578)

Zone event and run coalescing now races `requestAnimationFrame` and
`setTimeout`. There are tradeoffs to both functions and racing the two
gives us the benefits of both. This is explained in more detail in the
jsdoc comment. This change also aligns the timing of zone coalescing
with what will be used for zoneless.

BREAKING CHANGE: The exact timing of change detection execution when
using event or run coalescing with `NgZone` is now the first of either
`setTimeout` or `requestAnimationFrame`. Code which relies on this
timing (usually by accident) will need to be adjusted. If a callback
needs to execute after change detection, we recommend `afterNextRender`
instead of something like `setTimeout`.

fixes #54544
fixes #44314
fixes #39854 (unverified but seems likely)

PR Close #54578
  • Loading branch information
atscott committed Mar 13, 2024
1 parent 8cad4e8 commit 10c5cdb
Show file tree
Hide file tree
Showing 5 changed files with 138 additions and 116 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import {ApplicationRef} from '../../application/application_ref';
import {EnvironmentProviders, inject, Injectable, makeEnvironmentProviders} from '../../di';
import {PendingTasks} from '../../pending_tasks';
import {global} from '../../util/global';
import {getCallbackScheduler} from '../../util/callback_scheduler';
import {NgZone, NoopNgZone} from '../../zone/ng_zone';

import {ChangeDetectionScheduler, NotificationType} from './zoneless_scheduling';
Expand All @@ -20,6 +20,7 @@ class ChangeDetectionSchedulerImpl implements ChangeDetectionScheduler {
private taskService = inject(PendingTasks);
private pendingRenderTaskId: number|null = null;
private shouldRefreshViews = false;
private readonly schedule = getCallbackScheduler();

notify(type = NotificationType.RefreshViews): void {
// When the only source of notification is an afterRender hook will skip straight to the hooks
Expand All @@ -30,41 +31,11 @@ class ChangeDetectionSchedulerImpl implements ChangeDetectionScheduler {
}

this.pendingRenderTaskId = this.taskService.add();
this.raceTimeoutAndRequestAnimationFrame();
this.schedule(() => {
this.tick();
});
}

/**
* Run change detection after the first of setTimeout and requestAnimationFrame resolves.
*
* - `requestAnimationFrame` ensures that change detection runs ahead of a browser repaint.
* This ensures that the create and update passes of a change detection always happen
* in the same frame.
* - When the browser is resource-starved, `rAF` can execute _before_ a `setTimeout` because
* rendering is a very high priority process. This means that `setTimeout` cannot guarantee
* same-frame create and update pass, when `setTimeout` is used to schedule the update phase.
* - While `rAF` gives us the desirable same-frame updates, it has two limitations that
* prevent it from being used alone. First, it does not run in background tabs, which would
* prevent Angular from initializing an application when opened in a new tab (for example).
* Second, repeated calls to requestAnimationFrame will execute at the refresh rate of the
* hardware (~16ms for a 60Hz display). This would cause significant slowdown of tests that
* are written with several updates and asserts in the form of "update; await stable; assert;".
* - Both `setTimeout` and `rAF` are able to "coalesce" several events from a single user
* interaction into a single change detection. Importantly, this reduces view tree traversals when
* compared to an alternative timing mechanism like `queueMicrotask`, where change detection would
* then be interleaves between each event.
*
* By running change detection after the first of `setTimeout` and `rAF` to execute, we get the
* best of both worlds.
*/
private async raceTimeoutAndRequestAnimationFrame() {
const timeout = new Promise<void>(resolve => setTimeout(resolve));
const rAF = typeof global['requestAnimationFrame'] === 'function' ?
new Promise<void>(resolve => requestAnimationFrame(() => resolve())) :
null;
await Promise.race([timeout, rAF]);

this.tick();
}

private tick() {
try {
Expand Down
86 changes: 86 additions & 0 deletions packages/core/src/util/callback_scheduler.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

import {global} from './global';

/**
* Gets a scheduling function that runs the callback after the first of setTimeout and
* requestAnimationFrame resolves.
*
* - `requestAnimationFrame` ensures that change detection runs ahead of a browser repaint.
* This ensures that the create and update passes of a change detection always happen
* in the same frame.
* - When the browser is resource-starved, `rAF` can execute _before_ a `setTimeout` because
* rendering is a very high priority process. This means that `setTimeout` cannot guarantee
* same-frame create and update pass, when `setTimeout` is used to schedule the update phase.
* - While `rAF` gives us the desirable same-frame updates, it has two limitations that
* prevent it from being used alone. First, it does not run in background tabs, which would
* prevent Angular from initializing an application when opened in a new tab (for example).
* Second, repeated calls to requestAnimationFrame will execute at the refresh rate of the
* hardware (~16ms for a 60Hz display). This would cause significant slowdown of tests that
* are written with several updates and asserts in the form of "update; await stable; assert;".
* - Both `setTimeout` and `rAF` are able to "coalesce" several events from a single user
* interaction into a single change detection. Importantly, this reduces view tree traversals when
* compared to an alternative timing mechanism like `queueMicrotask`, where change detection would
* then be interleaves between each event.
*
* By running change detection after the first of `setTimeout` and `rAF` to execute, we get the
* best of both worlds.
*/
export function getCallbackScheduler() {
// Note: the `getNativeRequestAnimationFrame` is used in the `NgZone` class, but we cannot use the
// `inject` function. The `NgZone` instance may be created manually, and thus the injection
// context will be unavailable. This might be enough to check whether `requestAnimationFrame` is
// available because otherwise, we'll fall back to `setTimeout`.
const hasRequestAnimationFrame = typeof global['requestAnimationFrame'] === 'function';
let nativeRequestAnimationFrame =
hasRequestAnimationFrame ? global['requestAnimationFrame'] : null;
let nativeSetTimeout = global['setTimeout'];

if (typeof Zone !== 'undefined') {
// Note: zone.js sets original implementations on patched APIs behind the
// `__zone_symbol__OriginalDelegate` key (see `attachOriginToPatched`). Given the following
// example: `window.requestAnimationFrame.__zone_symbol__OriginalDelegate`; this would return an
// unpatched implementation of the `requestAnimationFrame`, which isn't intercepted by the
// Angular zone. We use the unpatched implementation to avoid another change detection when
// coalescing tasks.
const ORIGINAL_DELEGATE_SYMBOL = (Zone as any).__symbol__('OriginalDelegate');
if (nativeRequestAnimationFrame) {
nativeRequestAnimationFrame =
(nativeRequestAnimationFrame as any)[ORIGINAL_DELEGATE_SYMBOL] ??
nativeRequestAnimationFrame;
}
nativeSetTimeout = (nativeSetTimeout as any)[ORIGINAL_DELEGATE_SYMBOL] ?? nativeSetTimeout;
}

function scheduleCallback(callback: Function): {cancel: () => void} {
let executeCallback = true;
nativeSetTimeout(() => {
if (!executeCallback) {
return;
}
executeCallback = false;
callback();
});
nativeRequestAnimationFrame?.(() => {
if (!executeCallback) {
return;
}
executeCallback = false;
callback();
});

return {
cancel: () => {
executeCallback = false;
},
};
}

return scheduleCallback;
}
49 changes: 0 additions & 49 deletions packages/core/src/util/raf.ts

This file was deleted.

21 changes: 12 additions & 9 deletions packages/core/src/zone/ng_zone.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@

import {RuntimeError, RuntimeErrorCode} from '../errors';
import {EventEmitter} from '../event_emitter';
import {getCallbackScheduler} from '../util/callback_scheduler';
import {global} from '../util/global';
import {noop} from '../util/noop';
import {getNativeRequestAnimationFrame} from '../util/raf';

import {AsyncStackTaggingZoneSpec} from './async-stack-tagging';

Expand Down Expand Up @@ -164,8 +164,8 @@ export class NgZone {
self.shouldCoalesceEventChangeDetection =
!shouldCoalesceRunChangeDetection && shouldCoalesceEventChangeDetection;
self.shouldCoalesceRunChangeDetection = shouldCoalesceRunChangeDetection;
self.lastRequestAnimationFrameId = -1;
self.nativeRequestAnimationFrame = getNativeRequestAnimationFrame().nativeRequestAnimationFrame;
self.callbackScheduled = false;
self.scheduleCallback = getCallbackScheduler();
forkInnerZoneWithAngularBehavior(self);
}

Expand Down Expand Up @@ -273,7 +273,7 @@ interface NgZonePrivate extends NgZone {

hasPendingMacrotasks: boolean;
hasPendingMicrotasks: boolean;
lastRequestAnimationFrameId: number;
callbackScheduled: boolean;
/**
* A flag to indicate if NgZone is currently inside
* checkStable and to prevent re-entry. The flag is
Expand Down Expand Up @@ -325,7 +325,9 @@ interface NgZonePrivate extends NgZone {
*/
shouldCoalesceRunChangeDetection: boolean;

nativeRequestAnimationFrame: (callback: FrameRequestCallback) => number;
scheduleCallback: (callback: Function) => {
cancel: () => void
};

// Cache a "fake" top eventTask so you don't need to schedule a new task every
// time you run a `checkStable`.
Expand Down Expand Up @@ -379,10 +381,11 @@ function delayChangeDetectionForEvents(zone: NgZonePrivate) {
* so we also need to check the _nesting here to prevent multiple
* change detections.
*/
if (zone.isCheckStableRunning || zone.lastRequestAnimationFrameId !== -1) {
if (zone.isCheckStableRunning || zone.callbackScheduled) {
return;
}
zone.lastRequestAnimationFrameId = zone.nativeRequestAnimationFrame.call(global, () => {
zone.callbackScheduled = true;
zone.scheduleCallback.call(global, () => {
// This is a work around for https://github.com/angular/angular/issues/36839.
// The core issue is that when event coalescing is enabled it is possible for microtasks
// to get flushed too early (As is the case with `Promise.then`) between the
Expand All @@ -394,7 +397,7 @@ function delayChangeDetectionForEvents(zone: NgZonePrivate) {
// eventTask execution.
if (!zone.fakeTopEventTask) {
zone.fakeTopEventTask = Zone.root.scheduleEventTask('fakeTopEventTask', () => {
zone.lastRequestAnimationFrameId = -1;
zone.callbackScheduled = false;
updateMicroTaskStatus(zone);
zone.isCheckStableRunning = true;
checkStable(zone);
Expand Down Expand Up @@ -473,7 +476,7 @@ function forkInnerZoneWithAngularBehavior(zone: NgZonePrivate) {
function updateMicroTaskStatus(zone: NgZonePrivate) {
if (zone._hasPendingMicrotasks ||
((zone.shouldCoalesceEventChangeDetection || zone.shouldCoalesceRunChangeDetection) &&
zone.lastRequestAnimationFrameId !== -1)) {
zone.callbackScheduled === true)) {
zone.hasPendingMicrotasks = true;
} else {
zone.hasPendingMicrotasks = false;
Expand Down
Loading

0 comments on commit 10c5cdb

Please sign in to comment.