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

Performance issue with multiple concurrent uses of animationFrameScheduler #7017

Closed
fdeslandes-wk opened this issue Jul 8, 2022 · 2 comments · Fixed by #7059
Closed

Performance issue with multiple concurrent uses of animationFrameScheduler #7017

fdeslandes-wk opened this issue Jul 8, 2022 · 2 comments · Fixed by #7059
Assignees

Comments

@fdeslandes-wk
Copy link

Describe the bug

Since version 7.5.0, there is a huge performance loss with animationFrameScheduler when there are a lot of timers emitting concurrently using the scheduler. The problem can be painfully obvious on slower machines and was found in our application using Angular CDK virtual scroll.

There was no such performance issue in version 7.4.0. The problem have probably been created by this code change

The performance issue is big enough to prevent the migration of our application to RxJs 7.5 + as it makes some parts of it near unusable.

Expected behavior

Independant observables using the animationFrameScheduler should not have a big performance impact on each others.

Reproduction code

See the Stackblitz code.

See the result in the console using rxJs 7.5.0 vs using rxJs 7.4.0. The result is dependant on your computer's performance, but should be quite obvious regardless

Reproduction URL

https://stackblitz.com/edit/rxjs-afsbug?file=package.json,index.ts

Version

7.5.0

Environment

No response

Additional context

Works properly and is a lot more performant before 7.5.0

@FDIM
Copy link

FDIM commented Jul 26, 2022

Can confirm, we are downgrading to 7.4 after finding out that some parts of an application that relies on animationFrameScheduler is working only sometimes.

@benlesh
Copy link
Member

benlesh commented Sep 7, 2022

It looks like the issue is something to do with the addition of looping through all existing actions to see if the id is already scheduled in recycleActionId. cc @cartant who was doing some work in there.

recycleActionId is called every time schedule is called, (and possibly in a few other code paths), so we'll need to speed that up. Possibly with a Set or Map instead of looping over the actions.

@benlesh benlesh self-assigned this Sep 8, 2022
benlesh added a commit to benlesh/rxjs that referenced this issue Sep 8, 2022
…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 added a commit that referenced this issue Sep 25, 2022
…sapScheduler (#7059)

* refactor(schedulers): Appropriately type action ids

* 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

* chore: update api_guardian

* refactor(QueueAction): Have requestActionId return 0

Changes this to return `0` as a compromise given it was returning `void` in the past.
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this issue Oct 5, 2022
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#&#8203;757-httpsgithubcomreactivexrxjscompare756757-2022-09-25)

[Compare Source](ReactiveX/rxjs@7.5.6...7.5.7)

##### Bug Fixes

-   **schedulers:** improve performance of animationFrameScheduler and asapScheduler ([#&#8203;7059](ReactiveX/rxjs#7059)) ([c93aa60](ReactiveX/rxjs@c93aa60)), closes [#&#8203;7017](ReactiveX/rxjs#7017), related to [#&#8203;7018](ReactiveX/rxjs#7018) and [#&#8203;6674](ReactiveX/rxjs#6674)

##### Performance Improvements

-   **animationFrames:** uses fewer Subscription instances ([#&#8203;7060](ReactiveX/rxjs#7060)) ([2d57b38](ReactiveX/rxjs@2d57b38)), closes [#&#8203;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.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yMDIuNCIsInVwZGF0ZWRJblZlciI6IjMyLjIwOC4wIn0=-->

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>
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this issue Dec 23, 2022
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#&#8203;780-httpsgithubcomreactivexrxjscompare770780-2022-12-15)

[Compare Source](ReactiveX/rxjs@7.7.0...7.8.0)

##### Features

-   **buffer:** `closingNotifier` now supports any `ObservableInput` ([#&#8203;7073](ReactiveX/rxjs#7073)) ([61b877a](ReactiveX/rxjs@61b877a))
-   **delayWhen:** `delayWhen`'s `delayDurationSelector` now supports any `ObservableInput` ([#&#8203;7049](ReactiveX/rxjs#7049)) ([dfd95db](ReactiveX/rxjs@dfd95db))
-   **sequenceEqual:** `compareTo` now supports any `ObservableInput` ([#&#8203;7102](ReactiveX/rxjs#7102)) ([d501961](ReactiveX/rxjs@d501961))
-   **share:** `ShareConfig` factory properties now supports any `ObservableInput` ([#&#8203;7093](ReactiveX/rxjs#7093)) ([cc3995a](ReactiveX/rxjs@cc3995a))
-   **skipUntil:** `notifier` now supports any `ObservableInput` ([#&#8203;7091](ReactiveX/rxjs#7091)) ([60d6c40](ReactiveX/rxjs@60d6c40))
-   **window:** `windowBoundaries` now supports any `ObservableInput` ([#&#8203;7088](ReactiveX/rxjs#7088)) ([8c4347c](ReactiveX/rxjs@8c4347c))

### [`v7.7.0`](https://github.com/reactivex/rxjs/blob/HEAD/CHANGELOG.md#&#8203;770-httpsgithubcomreactivexrxjscompare760770-2022-12-15)

[Compare Source](ReactiveX/rxjs@7.6.0...7.7.0)

##### Features

-   **distinct:** `flush` argument now supports any `ObservableInput` ([#&#8203;7081](ReactiveX/rxjs#7081)) ([74c9ebd](ReactiveX/rxjs@74c9ebd))
-   **repeatWhen:** `notifier` supports `ObservableInput` ([#&#8203;7103](ReactiveX/rxjs#7103)) ([8f1b976](ReactiveX/rxjs@8f1b976))
-   **retryWhen:** `notifier` now supports any `ObservableInput` ([#&#8203;7105](ReactiveX/rxjs#7105)) ([794f806](ReactiveX/rxjs@794f806))
-   **sample:** `notifier` now supports any `ObservableInput` ([#&#8203;7104](ReactiveX/rxjs#7104)) ([b18c2eb](ReactiveX/rxjs@b18c2eb))

### [`v7.6.0`](https://github.com/reactivex/rxjs/blob/HEAD/CHANGELOG.md#&#8203;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 ([#&#8203;6718](ReactiveX/rxjs#6718)) ([af1a9f4](ReactiveX/rxjs@af1a9f4)), closes [#&#8203;6717](ReactiveX/rxjs#6717)

##### Features

-   **onErrorResumeNextWith:** renamed `onErrorResumeNext` and exported from the top level. (`onErrorResumeNext` operator is stil available, but deprecated) ([#&#8203;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 ([#&#8203;7059](ReactiveX/rxjs#7059)) ([c93aa60](ReactiveX/rxjs@c93aa60)), closes [#&#8203;7017](ReactiveX/rxjs#7017), related to [#&#8203;7018](ReactiveX/rxjs#7018) and [#&#8203;6674](ReactiveX/rxjs#6674)

##### Performance Improvements

-   **animationFrames:** uses fewer Subscription instances ([#&#8203;7060](ReactiveX/rxjs#7060)) ([2d57b38](ReactiveX/rxjs@2d57b38)), closes [#&#8203;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. ([#&#8203;7005](ReactiveX/rxjs#7005)) ([5d4c1d9](ReactiveX/rxjs@5d4c1d9))
-   **share & connect:** `share` and `connect` no longer bundle scheduling code by default ([#&#8203;6873](ReactiveX/rxjs#6873)) ([9948dc2](ReactiveX/rxjs@9948dc2)), closes [#&#8203;6872](ReactiveX/rxjs#6872)
-   **exhaustAll:** Result will now complete properly when flattening all synchronous observables. ([#&#8203;6911](ReactiveX/rxjs#6911)) ([3c1c6b8](ReactiveX/rxjs@3c1c6b8)), closes [#&#8203;6910](ReactiveX/rxjs#6910)
-   **TypeScript:** Now compatible with TypeScript 4.6 type checks ([#&#8203;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 ([#&#8203;6802](ReactiveX/rxjs#6802)) ([3750f75](ReactiveX/rxjs@3750f75))
-   **package:** add `require` export condition ([#&#8203;6821](ReactiveX/rxjs#6821)) ([c8955e4](ReactiveX/rxjs@c8955e4))
-   **timeout:** no longer will timeout when receiving the first value synchronously ([#&#8203;6865](ReactiveX/rxjs#6865)) ([2330c96](ReactiveX/rxjs@2330c96)), closes [#&#8203;6862](ReactiveX/rxjs#6862)

##### Performance Improvements

-   Don't clone observers unless you have to ([#&#8203;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`. ([#&#8203;6815](ReactiveX/rxjs#6815)) ([fb375a0](ReactiveX/rxjs@fb375a0)), closes [#&#8203;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 [#&#8203;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 ([#&#8203;6738](ReactiveX/rxjs#6738)) ([67cb317](ReactiveX/rxjs@67cb317)), closes [#&#8203;6536](ReactiveX/rxjs#6536)
-   **ajax:** crossDomain flag deprecated and properly reported to consumers ([#&#8203;6710](ReactiveX/rxjs#6710)) ([7fd0575](ReactiveX/rxjs@7fd0575)), closes [#&#8203;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. ([#&#8203;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.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC40OC4wIiwidXBkYXRlZEluVmVyIjoiMzQuNzAuNCJ9-->

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants