From bde6ad87c913eefaacfad0ca7e2d34c3435ab661 Mon Sep 17 00:00:00 2001 From: Ben Lesh Date: Wed, 7 Sep 2022 22:00:52 -0500 Subject: [PATCH 1/4] refactor(schedulers): Appropriately type action ids --- src/internal/scheduler/AnimationFrameAction.ts | 10 ++++++---- src/internal/scheduler/AsapAction.ts | 8 +++++--- src/internal/scheduler/AsyncAction.ts | 14 +++++++++----- src/internal/scheduler/AsyncScheduler.ts | 3 ++- src/internal/scheduler/QueueAction.ts | 16 ++++++++-------- src/internal/scheduler/VirtualTimeScheduler.ts | 7 ++++--- 6 files changed, 34 insertions(+), 24 deletions(-) diff --git a/src/internal/scheduler/AnimationFrameAction.ts b/src/internal/scheduler/AnimationFrameAction.ts index 771212f73d..f9b07be8ed 100644 --- a/src/internal/scheduler/AnimationFrameAction.ts +++ b/src/internal/scheduler/AnimationFrameAction.ts @@ -2,13 +2,14 @@ import { AsyncAction } from './AsyncAction'; import { AnimationFrameScheduler } from './AnimationFrameScheduler'; import { SchedulerAction } from '../types'; import { animationFrameProvider } from './animationFrameProvider'; +import { TimerHandle } from './timerHandle'; export class AnimationFrameAction extends AsyncAction { constructor(protected scheduler: AnimationFrameScheduler, protected work: (this: SchedulerAction, state?: T) => void) { super(scheduler, work); } - protected requestAsyncId(scheduler: AnimationFrameScheduler, id?: any, delay: number = 0): any { + protected requestAsyncId(scheduler: AnimationFrameScheduler, id?: TimerHandle, delay: number = 0): TimerHandle { // If delay is greater than 0, request as an async action. if (delay !== null && delay > 0) { return super.requestAsyncId(scheduler, id, delay); @@ -20,7 +21,8 @@ export class AnimationFrameAction extends AsyncAction { // the current animation frame request id. return scheduler._scheduled || (scheduler._scheduled = animationFrameProvider.requestAnimationFrame(() => scheduler.flush(undefined))); } - protected recycleAsyncId(scheduler: AnimationFrameScheduler, id?: any, delay: number = 0): any { + + protected recycleAsyncId(scheduler: AnimationFrameScheduler, id?: TimerHandle, delay: number = 0): TimerHandle | undefined { // If delay exists and is greater than 0, or if the delay is null (the // action wasn't rescheduled) but was originally scheduled as an async // action, then recycle as an async action. @@ -30,8 +32,8 @@ export class AnimationFrameAction extends AsyncAction { // If the scheduler queue has no remaining actions with the same async id, // cancel the requested animation frame and set the scheduled flag to // undefined so the next AnimationFrameAction will request its own. - if (!scheduler.actions.some((action) => action.id === id)) { - animationFrameProvider.cancelAnimationFrame(id); + if (id != null && !scheduler.actions.some((action) => action.id === id)) { + animationFrameProvider.cancelAnimationFrame(id as number); scheduler._scheduled = undefined; } // Return undefined so the action knows to request a new async id if it's rescheduled. diff --git a/src/internal/scheduler/AsapAction.ts b/src/internal/scheduler/AsapAction.ts index f8f5116e50..503d306b2c 100644 --- a/src/internal/scheduler/AsapAction.ts +++ b/src/internal/scheduler/AsapAction.ts @@ -2,13 +2,14 @@ import { AsyncAction } from './AsyncAction'; import { AsapScheduler } from './AsapScheduler'; import { SchedulerAction } from '../types'; import { immediateProvider } from './immediateProvider'; +import { TimerHandle } from './timerHandle'; export class AsapAction extends AsyncAction { constructor(protected scheduler: AsapScheduler, protected work: (this: SchedulerAction, state?: T) => void) { super(scheduler, work); } - protected requestAsyncId(scheduler: AsapScheduler, id?: any, delay: number = 0): any { + protected requestAsyncId(scheduler: AsapScheduler, id?: TimerHandle, delay: number = 0): TimerHandle { // If delay is greater than 0, request as an async action. if (delay !== null && delay > 0) { return super.requestAsyncId(scheduler, id, delay); @@ -20,7 +21,8 @@ export class AsapAction extends AsyncAction { // the current scheduled microtask id. return scheduler._scheduled || (scheduler._scheduled = immediateProvider.setImmediate(scheduler.flush.bind(scheduler, undefined))); } - protected recycleAsyncId(scheduler: AsapScheduler, id?: any, delay: number = 0): any { + + protected recycleAsyncId(scheduler: AsapScheduler, id?: TimerHandle, delay: number = 0): TimerHandle | undefined { // If delay exists and is greater than 0, or if the delay is null (the // action wasn't rescheduled) but was originally scheduled as an async // action, then recycle as an async action. @@ -30,7 +32,7 @@ export class AsapAction extends AsyncAction { // If the scheduler queue has no remaining actions with the same async id, // cancel the requested microtask and set the scheduled flag to undefined // so the next AsapAction will request its own. - if (!scheduler.actions.some((action) => action.id === id)) { + if (id != null && !scheduler.actions.some((action) => action.id === id)) { immediateProvider.clearImmediate(id); scheduler._scheduled = undefined; } diff --git a/src/internal/scheduler/AsyncAction.ts b/src/internal/scheduler/AsyncAction.ts index f7d600472e..d7ffe51185 100644 --- a/src/internal/scheduler/AsyncAction.ts +++ b/src/internal/scheduler/AsyncAction.ts @@ -4,9 +4,10 @@ import { Subscription } from '../Subscription'; import { AsyncScheduler } from './AsyncScheduler'; import { intervalProvider } from './intervalProvider'; import { arrRemove } from '../util/arrRemove'; +import { TimerHandle } from './timerHandle'; export class AsyncAction extends Action { - public id: any; + public id: TimerHandle | undefined; public state?: T; // @ts-ignore: Property has no initializer and is not definitely assigned public delay: number; @@ -58,23 +59,26 @@ export class AsyncAction extends Action { this.delay = delay; // If this action has already an async Id, don't request a new one. - this.id = this.id || this.requestAsyncId(scheduler, this.id, delay); + this.id = this.id ?? this.requestAsyncId(scheduler, this.id, delay); return this; } - protected requestAsyncId(scheduler: AsyncScheduler, _id?: any, delay: number = 0): any { + protected requestAsyncId(scheduler: AsyncScheduler, _id?: TimerHandle, delay: number = 0): TimerHandle { return intervalProvider.setInterval(scheduler.flush.bind(scheduler, this), delay); } - protected recycleAsyncId(_scheduler: AsyncScheduler, id: any, delay: number | null = 0): any { + protected recycleAsyncId(_scheduler: AsyncScheduler, id?: TimerHandle, delay: number | null = 0): TimerHandle | undefined { // If this action is rescheduled with the same delay time, don't clear the interval id. if (delay != null && this.delay === delay && this.pending === false) { return id; } // Otherwise, if the action's delay time is different from the current delay, // or the action has been rescheduled before it's executed, clear the interval id - intervalProvider.clearInterval(id); + if (id != null) { + intervalProvider.clearInterval(id); + } + return undefined; } diff --git a/src/internal/scheduler/AsyncScheduler.ts b/src/internal/scheduler/AsyncScheduler.ts index 518ab24b21..fc04d66236 100644 --- a/src/internal/scheduler/AsyncScheduler.ts +++ b/src/internal/scheduler/AsyncScheduler.ts @@ -1,6 +1,7 @@ import { Scheduler } from '../Scheduler'; import { Action } from './Action'; import { AsyncAction } from './AsyncAction'; +import { TimerHandle } from './timerHandle'; export class AsyncScheduler extends Scheduler { public actions: Array> = []; @@ -18,7 +19,7 @@ export class AsyncScheduler extends Scheduler { * @type {any} * @internal */ - public _scheduled: any = undefined; + public _scheduled: TimerHandle | undefined; constructor(SchedulerAction: typeof Action, now: () => number = Scheduler.now) { super(SchedulerAction, now); diff --git a/src/internal/scheduler/QueueAction.ts b/src/internal/scheduler/QueueAction.ts index 46fba03ac7..9758f6523a 100644 --- a/src/internal/scheduler/QueueAction.ts +++ b/src/internal/scheduler/QueueAction.ts @@ -2,11 +2,10 @@ import { AsyncAction } from './AsyncAction'; import { Subscription } from '../Subscription'; import { QueueScheduler } from './QueueScheduler'; import { SchedulerAction } from '../types'; +import { TimerHandle } from './timerHandle'; export class QueueAction extends AsyncAction { - - constructor(protected scheduler: QueueScheduler, - protected work: (this: SchedulerAction, state?: T) => void) { + constructor(protected scheduler: QueueScheduler, protected work: (this: SchedulerAction, state?: T) => void) { super(scheduler, work); } @@ -21,12 +20,10 @@ export class QueueAction extends AsyncAction { } public execute(state: T, delay: number): any { - return (delay > 0 || this.closed) ? - super.execute(state, delay) : - this._execute(state, delay) ; + return delay > 0 || this.closed ? super.execute(state, delay) : this._execute(state, delay); } - protected requestAsyncId(scheduler: QueueScheduler, id?: any, delay: number = 0): any { + protected requestAsyncId(scheduler: QueueScheduler, id?: TimerHandle, delay: number = 0): TimerHandle { // If delay exists and is greater than 0, or if the delay is null (the // action wasn't rescheduled) but was originally scheduled as an async // action, then recycle as an async action. @@ -34,7 +31,10 @@ export class QueueAction extends AsyncAction { if ((delay != null && delay > 0) || (delay == null && this.delay > 0)) { return super.requestAsyncId(scheduler, id, delay); } + // Otherwise flush the scheduler starting with this action. - return scheduler.flush(this); + scheduler.flush(this); + + return 1; } } diff --git a/src/internal/scheduler/VirtualTimeScheduler.ts b/src/internal/scheduler/VirtualTimeScheduler.ts index 34ce4552c3..870a144a76 100644 --- a/src/internal/scheduler/VirtualTimeScheduler.ts +++ b/src/internal/scheduler/VirtualTimeScheduler.ts @@ -2,6 +2,7 @@ import { AsyncAction } from './AsyncAction'; import { Subscription } from '../Subscription'; import { AsyncScheduler } from './AsyncScheduler'; import { SchedulerAction } from '../types'; +import { TimerHandle } from './timerHandle'; export class VirtualTimeScheduler extends AsyncScheduler { /** @deprecated Not used in VirtualTimeScheduler directly. Will be removed in v8. */ @@ -92,15 +93,15 @@ export class VirtualAction extends AsyncAction { } } - protected requestAsyncId(scheduler: VirtualTimeScheduler, id?: any, delay: number = 0): any { + protected requestAsyncId(scheduler: VirtualTimeScheduler, id?: any, delay: number = 0): TimerHandle { this.delay = scheduler.frame + delay; const { actions } = scheduler; actions.push(this); (actions as Array>).sort(VirtualAction.sortActions); - return true; + return 1; } - protected recycleAsyncId(scheduler: VirtualTimeScheduler, id?: any, delay: number = 0): any { + protected recycleAsyncId(scheduler: VirtualTimeScheduler, id?: any, delay: number = 0): TimerHandle | undefined { return undefined; } From 4759a8c2beff62ff60368d05f125f067030cf3de Mon Sep 17 00:00:00 2001 From: Ben Lesh Date: Thu, 8 Sep 2022 18:34:42 -0500 Subject: [PATCH 2/4] fix(animationFrameScheduler): improve performance of animationFrameScheduler + Changes the check for existing action ids to simply check the last action in the queue to see if its id matches. Previously we were doing an O(n) loop on each execution of an action to check to see if the scheduling id needed to be recycled. This is problematic in AsapScheduler and AnimationFrameScheduler, where we're not reusing an interval. Since AsapScheduler and AnimationFrameScheduler reuse the most recent action id until their scheduled microtask or animation frame fires, the last action in the actions queue array is all we really need to check (rather than checking them all with `some`). O(1) vs O(n). + Refactors a weird conditional gaff from `if ((X && A) || (!X && B))` to just be `if (X ? A : B)` resolves #7017 related #7018 related #6674 --- src/internal/scheduler/AnimationFrameAction.ts | 5 +++-- src/internal/scheduler/AsapAction.ts | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/internal/scheduler/AnimationFrameAction.ts b/src/internal/scheduler/AnimationFrameAction.ts index f9b07be8ed..f9c8f8e39d 100644 --- a/src/internal/scheduler/AnimationFrameAction.ts +++ b/src/internal/scheduler/AnimationFrameAction.ts @@ -26,13 +26,14 @@ export class AnimationFrameAction extends AsyncAction { // If delay exists and is greater than 0, or if the delay is null (the // action wasn't rescheduled) but was originally scheduled as an async // action, then recycle as an async action. - if ((delay != null && delay > 0) || (delay == null && this.delay > 0)) { + if (delay != null ? delay > 0 : this.delay > 0) { return super.recycleAsyncId(scheduler, id, delay); } // If the scheduler queue has no remaining actions with the same async id, // cancel the requested animation frame and set the scheduled flag to // undefined so the next AnimationFrameAction will request its own. - if (id != null && !scheduler.actions.some((action) => action.id === id)) { + const { actions } = scheduler; + if (id != null && actions[actions.length - 1]?.id !== id) { animationFrameProvider.cancelAnimationFrame(id as number); scheduler._scheduled = undefined; } diff --git a/src/internal/scheduler/AsapAction.ts b/src/internal/scheduler/AsapAction.ts index 503d306b2c..bd4b8697c3 100644 --- a/src/internal/scheduler/AsapAction.ts +++ b/src/internal/scheduler/AsapAction.ts @@ -26,13 +26,14 @@ export class AsapAction extends AsyncAction { // If delay exists and is greater than 0, or if the delay is null (the // action wasn't rescheduled) but was originally scheduled as an async // action, then recycle as an async action. - if ((delay != null && delay > 0) || (delay == null && this.delay > 0)) { + if (delay != null ? delay > 0 : this.delay > 0) { return super.recycleAsyncId(scheduler, id, delay); } // If the scheduler queue has no remaining actions with the same async id, // cancel the requested microtask and set the scheduled flag to undefined // so the next AsapAction will request its own. - if (id != null && !scheduler.actions.some((action) => action.id === id)) { + const { actions } = scheduler; + if (id != null && actions[actions.length - 1]?.id !== id) { immediateProvider.clearImmediate(id); scheduler._scheduled = undefined; } From fde8f5fda72943deba0fa9d88d9d7b367fe8b74d Mon Sep 17 00:00:00 2001 From: Ben Lesh Date: Thu, 8 Sep 2022 19:05:10 -0500 Subject: [PATCH 3/4] chore: update api_guardian --- api_guard/dist/types/index.d.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api_guard/dist/types/index.d.ts b/api_guard/dist/types/index.d.ts index 1a14efdf38..387e385a6b 100644 --- a/api_guard/dist/types/index.d.ts +++ b/api_guard/dist/types/index.d.ts @@ -858,8 +858,8 @@ export declare class VirtualAction extends AsyncAction { protected work: (this: SchedulerAction, state?: T) => void; constructor(scheduler: VirtualTimeScheduler, work: (this: SchedulerAction, state?: T) => void, index?: number); protected _execute(state: T, delay: number): any; - protected recycleAsyncId(scheduler: VirtualTimeScheduler, id?: any, delay?: number): any; - protected requestAsyncId(scheduler: VirtualTimeScheduler, id?: any, delay?: number): any; + protected recycleAsyncId(scheduler: VirtualTimeScheduler, id?: any, delay?: number): TimerHandle | undefined; + protected requestAsyncId(scheduler: VirtualTimeScheduler, id?: any, delay?: number): TimerHandle; schedule(state?: T, delay?: number): Subscription; } From d415101532a24aa845d1c98b632e948a5f38317e Mon Sep 17 00:00:00 2001 From: Ben Lesh Date: Fri, 9 Sep 2022 09:18:44 -0500 Subject: [PATCH 4/4] refactor(QueueAction): Have requestActionId return 0 Changes this to return `0` as a compromise given it was returning `void` in the past. --- src/internal/scheduler/QueueAction.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/internal/scheduler/QueueAction.ts b/src/internal/scheduler/QueueAction.ts index 9758f6523a..9016edbc56 100644 --- a/src/internal/scheduler/QueueAction.ts +++ b/src/internal/scheduler/QueueAction.ts @@ -35,6 +35,10 @@ export class QueueAction extends AsyncAction { // Otherwise flush the scheduler starting with this action. scheduler.flush(this); - return 1; + // HACK: In the past, this was returning `void`. However, `void` isn't a valid + // `TimerHandle`, and generally the return value here isn't really used. So the + // compromise is to return `0` which is both "falsy" and a valid `TimerHandle`, + // as opposed to refactoring every other instanceo of `requestAsyncId`. + return 0; } }