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

benlesh · 2022-09-08T23:58:16Z

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)
Appropriately types action ids

resolves #7017
related #7018
related #6674

…heduler + 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 ReactiveX#7017 related ReactiveX#7018 related ReactiveX#6674

benlesh · 2022-09-08T23:58:58Z

src/internal/scheduler/AnimationFrameAction.ts


 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 {


Most changed lines are just around typing these ids appropriately.

benlesh · 2022-09-09T00:00:00Z

src/internal/scheduler/AnimationFrameAction.ts

    // 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) {


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

benlesh · 2022-09-09T00:00:08Z

src/internal/scheduler/AsapAction.ts

    // 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) {


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

benlesh · 2022-09-09T00:01:21Z

src/internal/scheduler/AsapAction.ts

      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) {


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.

benlesh · 2022-09-09T00:02:01Z

src/internal/scheduler/AnimationFrameAction.ts

-    if (!scheduler.actions.some((action) => action.id === id)) {
-      animationFrameProvider.cancelAnimationFrame(id);
+    const { actions } = scheduler;
+    if (id != null && actions[actions.length - 1]?.id !== id) {


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.

benlesh · 2022-09-09T00:02:34Z

src/internal/scheduler/AsyncAction.ts

    // 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);


This was being called with undefined at times before.

benlesh · 2022-09-09T00:03:36Z

src/internal/scheduler/QueueAction.ts

-    return scheduler.flush(this);
+    scheduler.flush(this);
+
+    return 1;


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.

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

rxjs/src/internal/scheduler/QueueScheduler.ts

Lines 3 to 4 in 4afbc16

export class QueueScheduler extends AsyncScheduler {

}

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

rxjs/src/internal/scheduler/AsyncScheduler.ts

Line 27 in 4afbc16

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

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

cartant

I've read through this and checking the id of only the last element in the actions array makes sense. I'm not sure about returning 1 where the return value of flush used to be returned, though - I left a comment. I'll have another look at it in the morning. A little too tired to think that one through ATM.

cartant · 2022-09-09T10:17:36Z

src/internal/scheduler/QueueAction.ts

-    return scheduler.flush(this);
+    scheduler.flush(this);
+
+    return 1;


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

rxjs/src/internal/scheduler/QueueScheduler.ts

Lines 3 to 4 in 4afbc16

export class QueueScheduler extends AsyncScheduler {

}

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

rxjs/src/internal/scheduler/AsyncScheduler.ts

Line 27 in 4afbc16

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

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

cartant · 2022-09-09T13:12:17Z

src/internal/scheduler/AnimationFrameAction.ts

-      animationFrameProvider.cancelAnimationFrame(id);
+    const { actions } = scheduler;
+    if (id != null && actions[actions.length - 1]?.id !== id) {
+      animationFrameProvider.cancelAnimationFrame(id as number);
      scheduler._scheduled = undefined;


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:

rxjs/src/internal/scheduler/AsyncAction.ts

Line 140 in 4afbc16

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

benlesh · 2022-09-09T14:09:09Z

@cartant on QueueAction requestAsyncId returning 1. It was actually returning void before, now that I look at it. However, that's not a valid TimerHandle and the (obviously crazy) API is expecting a TimerHandle to be returned, even though it's not really using it in this case. I could change it to 0 so that it's a valid TimerHandle, but also falsy. OR I could go through and make all of the other requestAsyncId overrides return TimerHandle | undefined. Personally, I think that returning 0 and adding a comment might be the more prudent action here, so I'll preemptively do that.

This whole area of the library isn't long for this world. I want to deprecate it and replace it in version 8 with it fully phased out by version 9.

Changes this to return `0` as a compromise given it was returning `void` in the past.

cartant

Personally, I think that returning 0 and adding a comment might be the more prudent action here, so I'll preemptively do that.

I think that will be fine. Looking at the code, the worst thing that seems possible is that if schedule is called on the action and a delay is specified, clearInterval will be called with 0, here:

rxjs/src/internal/scheduler/AsyncAction.ts

Line 77 in 4afbc16

intervalProvider.clearInterval(id);

because this 0 will pass this check:

rxjs/src/internal/scheduler/AsyncAction.ts

Lines 51 to 53 in 4afbc16

    
           if (id != null) { 
        
             this.id = this.recycleAsyncId(scheduler, id, delay); 
        
           }

And that should be no big deal.

Later, I'll have a look at the possible problem I mentioned here, but that shouldn't hold up this PR.

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [rxjs](https://rxjs.dev) ([source](https://github.com/reactivex/rxjs)) | dependencies | patch | [`7.5.6` -> `7.5.7`](https://renovatebot.com/diffs/npm/rxjs/7.5.6/7.5.7) | --- ### Release Notes <details> <summary>reactivex/rxjs</summary> ### [`v7.5.7`](https://github.com/reactivex/rxjs/blob/HEAD/CHANGELOG.md#757-httpsgithubcomreactivexrxjscompare756757-2022-09-25) [Compare Source](ReactiveX/rxjs@7.5.6...7.5.7) ##### Bug Fixes - **schedulers:** improve performance of animationFrameScheduler and asapScheduler ([#7059](ReactiveX/rxjs#7059)) ([c93aa60](ReactiveX/rxjs@c93aa60)), closes [#7017](ReactiveX/rxjs#7017), related to [#7018](ReactiveX/rxjs#7018) and [#6674](ReactiveX/rxjs#6674) ##### Performance Improvements - **animationFrames:** uses fewer Subscription instances ([#7060](ReactiveX/rxjs#7060)) ([2d57b38](ReactiveX/rxjs@2d57b38)), closes [#7018](ReactiveX/rxjs#7018) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1562 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [rxjs](https://rxjs.dev) ([source](https://github.com/reactivex/rxjs)) | dependencies | minor | [`7.5.7` -> `7.8.0`](https://renovatebot.com/diffs/npm/rxjs/7.5.7/7.8.0) | --- ### Release Notes <details> <summary>reactivex/rxjs</summary> ### [`v7.8.0`](https://github.com/reactivex/rxjs/blob/HEAD/CHANGELOG.md#780-httpsgithubcomreactivexrxjscompare770780-2022-12-15) [Compare Source](ReactiveX/rxjs@7.7.0...7.8.0) ##### Features - **buffer:** `closingNotifier` now supports any `ObservableInput` ([#7073](ReactiveX/rxjs#7073)) ([61b877a](ReactiveX/rxjs@61b877a)) - **delayWhen:** `delayWhen`'s `delayDurationSelector` now supports any `ObservableInput` ([#7049](ReactiveX/rxjs#7049)) ([dfd95db](ReactiveX/rxjs@dfd95db)) - **sequenceEqual:** `compareTo` now supports any `ObservableInput` ([#7102](ReactiveX/rxjs#7102)) ([d501961](ReactiveX/rxjs@d501961)) - **share:** `ShareConfig` factory properties now supports any `ObservableInput` ([#7093](ReactiveX/rxjs#7093)) ([cc3995a](ReactiveX/rxjs@cc3995a)) - **skipUntil:** `notifier` now supports any `ObservableInput` ([#7091](ReactiveX/rxjs#7091)) ([60d6c40](ReactiveX/rxjs@60d6c40)) - **window:** `windowBoundaries` now supports any `ObservableInput` ([#7088](ReactiveX/rxjs#7088)) ([8c4347c](ReactiveX/rxjs@8c4347c)) ### [`v7.7.0`](https://github.com/reactivex/rxjs/blob/HEAD/CHANGELOG.md#770-httpsgithubcomreactivexrxjscompare760770-2022-12-15) [Compare Source](ReactiveX/rxjs@7.6.0...7.7.0) ##### Features - **distinct:** `flush` argument now supports any `ObservableInput` ([#7081](ReactiveX/rxjs#7081)) ([74c9ebd](ReactiveX/rxjs@74c9ebd)) - **repeatWhen:** `notifier` supports `ObservableInput` ([#7103](ReactiveX/rxjs#7103)) ([8f1b976](ReactiveX/rxjs@8f1b976)) - **retryWhen:** `notifier` now supports any `ObservableInput` ([#7105](ReactiveX/rxjs#7105)) ([794f806](ReactiveX/rxjs@794f806)) - **sample:** `notifier` now supports any `ObservableInput` ([#7104](ReactiveX/rxjs#7104)) ([b18c2eb](ReactiveX/rxjs@b18c2eb)) ### [`v7.6.0`](https://github.com/reactivex/rxjs/blob/HEAD/CHANGELOG.md#760-httpsgithubcomreactivexrxjscompare757760-2022-12-03) [Compare Source](ReactiveX/rxjs@7.5.7...7.6.0) ##### Bug Fixes - **schedulers:** no longer cause TypeScript build failures when Node types aren't included ([c1a07b7](ReactiveX/rxjs@c1a07b7)) - **types:** Improved subscribe and tap type overloads ([#6718](ReactiveX/rxjs#6718)) ([af1a9f4](ReactiveX/rxjs@af1a9f4)), closes [#6717](ReactiveX/rxjs#6717) ##### Features - **onErrorResumeNextWith:** renamed `onErrorResumeNext` and exported from the top level. (`onErrorResumeNext` operator is stil available, but deprecated) ([#6755](ReactiveX/rxjs#6755)) ([51e3b2c](ReactiveX/rxjs@51e3b2c)) #### [7.5.7](ReactiveX/rxjs@7.5.6...7.5.7) (2022-09-25) ##### Bug Fixes - **schedulers:** improve performance of animationFrameScheduler and asapScheduler ([#7059](ReactiveX/rxjs#7059)) ([c93aa60](ReactiveX/rxjs@c93aa60)), closes [#7017](ReactiveX/rxjs#7017), related to [#7018](ReactiveX/rxjs#7018) and [#6674](ReactiveX/rxjs#6674) ##### Performance Improvements - **animationFrames:** uses fewer Subscription instances ([#7060](ReactiveX/rxjs#7060)) ([2d57b38](ReactiveX/rxjs@2d57b38)), closes [#7018](ReactiveX/rxjs#7018) #### [7.5.6](ReactiveX/rxjs@7.5.5...7.5.6) (2022-07-11) ##### Bug Fixes - **share:** No longer results in a bad-state observable in an edge case where a synchronous source was shared and refCounted, and the result is subscribed to twice in a row synchronously. ([#7005](ReactiveX/rxjs#7005)) ([5d4c1d9](ReactiveX/rxjs@5d4c1d9)) - **share & connect:** `share` and `connect` no longer bundle scheduling code by default ([#6873](ReactiveX/rxjs#6873)) ([9948dc2](ReactiveX/rxjs@9948dc2)), closes [#6872](ReactiveX/rxjs#6872) - **exhaustAll:** Result will now complete properly when flattening all synchronous observables. ([#6911](ReactiveX/rxjs#6911)) ([3c1c6b8](ReactiveX/rxjs@3c1c6b8)), closes [#6910](ReactiveX/rxjs#6910) - **TypeScript:** Now compatible with TypeScript 4.6 type checks ([#6895](ReactiveX/rxjs#6895)) ([fce9aa1](ReactiveX/rxjs@fce9aa1)) #### [7.5.5](ReactiveX/rxjs@7.5.4...7.5.5) (2022-03-08) ##### Bug Fixes - **package:** add types to exports ([#6802](ReactiveX/rxjs#6802)) ([3750f75](ReactiveX/rxjs@3750f75)) - **package:** add `require` export condition ([#6821](ReactiveX/rxjs#6821)) ([c8955e4](ReactiveX/rxjs@c8955e4)) - **timeout:** no longer will timeout when receiving the first value synchronously ([#6865](ReactiveX/rxjs#6865)) ([2330c96](ReactiveX/rxjs@2330c96)), closes [#6862](ReactiveX/rxjs#6862) ##### Performance Improvements - Don't clone observers unless you have to ([#6842](ReactiveX/rxjs#6842)) ([3289d20](ReactiveX/rxjs@3289d20)) #### [7.5.4](ReactiveX/rxjs@7.5.3...7.5.4) (2022-02-09) ##### Performance Improvements - removed code that would `bind` functions passed with observers to `subscribe`. ([#6815](ReactiveX/rxjs#6815)) ([fb375a0](ReactiveX/rxjs@fb375a0)), closes [#6783](ReactiveX/rxjs#6783) #### [7.5.3](ReactiveX/rxjs@7.5.2...7.5.3) (2022-02-08) ##### Bug Fixes - **subscribe:** allow interop with Monio and other libraries that patch function bind ([0ab91eb](ReactiveX/rxjs@0ab91eb)), closes [#6783](ReactiveX/rxjs#6783) #### [7.5.2](ReactiveX/rxjs@7.5.1...7.5.2) (2022-01-11) ##### Bug Fixes - operators that ignore input values now use `unknown` rather than `any`, which should resolve issues with eslint no-unsafe-argument ([#6738](ReactiveX/rxjs#6738)) ([67cb317](ReactiveX/rxjs@67cb317)), closes [#6536](ReactiveX/rxjs#6536) - **ajax:** crossDomain flag deprecated and properly reported to consumers ([#6710](ReactiveX/rxjs#6710)) ([7fd0575](ReactiveX/rxjs@7fd0575)), closes [#6663](ReactiveX/rxjs#6663) #### [7.5.1](ReactiveX/rxjs@7.5.0...7.5.1) (2021-12-28) ##### Bug Fixes - export supporting interfaces from top-level `rxjs` site. ([#6733](ReactiveX/rxjs#6733)) ([299a1e1](ReactiveX/rxjs@299a1e1)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1668 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

benlesh added 2 commits September 7, 2022 22:06

refactor(schedulers): Appropriately type action ids

bde6ad8

benlesh requested a review from cartant September 8, 2022 23:58

benlesh commented Sep 8, 2022

View reviewed changes

benlesh commented Sep 9, 2022

View reviewed changes

chore: update api_guardian

fde8f5f

cartant reviewed Sep 9, 2022

View reviewed changes

refactor(QueueAction): Have requestActionId return 0

d415101

Changes this to return `0` as a compromise given it was returning `void` in the past.

cartant approved these changes Sep 10, 2022

View reviewed changes

benlesh merged commit c93aa60 into ReactiveX:master Sep 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

benlesh commented Sep 8, 2022

benlesh Sep 8, 2022

benlesh Sep 9, 2022

benlesh Sep 9, 2022

benlesh Sep 9, 2022

benlesh Sep 9, 2022

benlesh Sep 9, 2022

benlesh Sep 9, 2022

cartant Sep 9, 2022

cartant left a comment

cartant Sep 9, 2022

cartant Sep 9, 2022

benlesh commented Sep 9, 2022

cartant left a comment •

edited

Loading

	if (id != null) {
	this.id = this.recycleAsyncId(scheduler, id, delay);
	}

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

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

Conversation

benlesh commented Sep 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cartant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benlesh commented Sep 9, 2022

cartant left a comment • edited Loading

Choose a reason for hiding this comment

cartant left a comment •

edited

Loading