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

fix(schedulers): improve performance of animationFrameScheduler and asapScheduler #7059

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions src/internal/scheduler/AnimationFrameAction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<T> extends AsyncAction<T> {
constructor(protected scheduler: AnimationFrameScheduler, protected work: (this: SchedulerAction<T>, 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 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most changed lines are just around typing these ids appropriately.

// If delay is greater than 0, request as an async action.
if (delay !== null && delay > 0) {
return super.requestAsyncId(scheduler, id, delay);
Expand All @@ -20,18 +21,20 @@ export class AnimationFrameAction<T> extends AsyncAction<T> {
// 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.
if ((delay != null && delay > 0) || (delay == null && this.delay > 0)) {
if (delay != null ? delay > 0 : this.delay > 0) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplify conditional ((X && A) || (!X && B)) to (X ? A : B)

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 (!scheduler.actions.some((action) => action.id === id)) {
animationFrameProvider.cancelAnimationFrame(id);
const { actions } = scheduler;
if (id != null && actions[actions.length - 1]?.id !== id) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perf improvement here. arr.some(x => x.id === id) to arr[arr.length - 1]?.id === id. Which is O(n) to O(1) in hot path code.

animationFrameProvider.cancelAnimationFrame(id as number);
scheduler._scheduled = undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't relate to a change that's made in this PR, but I'm not at all sure that setting _scheduled to undefined here is correct (this happens in the AsapScheduler, too). I think this was done - pretty sure, by me - when looking at explicit unsubscriptions of actions - because the unsubscribe calls recycleAsyncId:

this.id = this.recycleAsyncId(scheduler, id, null);

But if we've come into this block under different circumstances, it might make no sense. E.g. if there's an action at the end of the array that was scheduled from within a flush - and has a different id - there'll have been a corresponding requestAnimationFrame call and _scheduled will be holding the handle for that call.

This stuff is so complicated. Will look again at this tomorrow, but I have a bad feeling it's wrong. (Again, not a change in this PR; it was already like this.)

}
// Return undefined so the action knows to request a new async id if it's rescheduled.
Expand Down
11 changes: 7 additions & 4 deletions src/internal/scheduler/AsapAction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<T> extends AsyncAction<T> {
constructor(protected scheduler: AsapScheduler, protected work: (this: SchedulerAction<T>, 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);
Expand All @@ -20,17 +21,19 @@ export class AsapAction<T> extends AsyncAction<T> {
// 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.
if ((delay != null && delay > 0) || (delay == null && this.delay > 0)) {
if (delay != null ? delay > 0 : this.delay > 0) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplify conditional ((X && A) || (!X && B)) to (X ? A : B)

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 (!scheduler.actions.some((action) => action.id === id)) {
const { actions } = scheduler;
if (id != null && actions[actions.length - 1]?.id !== id) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perf improvement here. arr.some(x => x.id === id) to arr[arr.length - 1]?.id === id. Which is O(n) to O(1) in hot path code.

immediateProvider.clearImmediate(id);
scheduler._scheduled = undefined;
}
Expand Down
14 changes: 9 additions & 5 deletions src/internal/scheduler/AsyncAction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<T> extends Action<T> {
public id: any;
public id: TimerHandle | undefined;
public state?: T;
// @ts-ignore: Property has no initializer and is not definitely assigned
public delay: number;
Expand Down Expand Up @@ -58,23 +59,26 @@ export class AsyncAction<T> extends Action<T> {

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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was being called with undefined at times before.

}

return undefined;
}

Expand Down
3 changes: 2 additions & 1 deletion src/internal/scheduler/AsyncScheduler.ts
Original file line number Diff line number Diff line change
@@ -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<AsyncAction<any>> = [];
Expand All @@ -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);
Expand Down
16 changes: 8 additions & 8 deletions src/internal/scheduler/QueueAction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<T> extends AsyncAction<T> {

constructor(protected scheduler: QueueScheduler,
protected work: (this: SchedulerAction<T>, state?: T) => void) {
constructor(protected scheduler: QueueScheduler, protected work: (this: SchedulerAction<T>, state?: T) => void) {
super(scheduler, work);
}

Expand All @@ -21,20 +20,21 @@ export class QueueAction<T> extends AsyncAction<T> {
}

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.

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;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, this was returning true, which isn't a TimerHandle type. I changed it to a 1. Perhaps less memory efficient, but it's code-efficient and in-character for a TimerHandle.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this really returning true? AFAICT, QueueScheduler has no flush:

export class QueueScheduler extends AsyncScheduler {
}

And the flush in AsyncScheduler - which QueueScheduler extends - returns void:

public flush(action: AsyncAction<any>): void {

IDK what the consequences of returning something truthy will be when it previously appeared to return something falsy.

}
}
7 changes: 4 additions & 3 deletions src/internal/scheduler/VirtualTimeScheduler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down Expand Up @@ -92,15 +93,15 @@ export class VirtualAction<T> extends AsyncAction<T> {
}
}

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<VirtualAction<T>>).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;
}

Expand Down