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

Stop RAF update loop when there are no pending tasks #1786

Merged
merged 9 commits into from
Dec 16, 2021

Conversation

srubin
Copy link
Contributor

@srubin srubin commented Dec 14, 2021

Why

A quick test in Chrome shows that a no-op requestAnimationFrame update loop idles between 5-10% CPU usage (codesandbox).

image

@react-spring/rafz and react-spring are susceptible to this behavior: once a task is scheduled with rafz, the update loop never stops even if there is never any other work scheduled. See this bug, which shows an example of this issue: #1732

What

This PR stops the raf update loop if there are no pending tasks. It does so using a task counting variable that was already present in the code but only used for testing. I added unit tests to verify that the update loop is running when necessary and stopped when able.

I also ran the code sample from #1732 (and on another codebase) with this code change and verified that the raf update loop is not running indefinitely:

image

(left, before hitting "Destroy AnimationComponent"; right: after hitting "Destroy AnimationComponent")

I could use some guidance as to how to verify that this change is not negatively impacting or regressing anything else in the library.

Checklist

  • Documentation updated (because this is just an implementation detail of rafz, is documentation required?)
  • Demo added (in the form of unit tests, I guess?)
  • Ready to be merged

Sorry, something went wrong.

david-buck and others added 4 commits December 11, 2021 16:54

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Update "Transitioning between routes" example to use latest React Router syntax

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
Co-authored-by: Josh <37798644+joshuaellis@users.noreply.github.com>
@vercel
Copy link

vercel bot commented Dec 14, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pmndrs/react-spring-io/4Yzat6kRQJVL4VnzBrjvCG4wb1Vd
✅ Preview: https://react-spring-io-git-fork-descriptinc-sr-stop-raf-pmndrs.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 14, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 6ada3cf:

Sandbox Source
spring-card Configuration
spring-goo-blobs Configuration
spring-flip-card Configuration
pmndrs/react-spring Configuration
spring-draggable-list Configuration
pmndrs/react-spring Configuration
spring-viewpager Configuration
pmndrs/react-spring Configuration
spring-image-fade Configuration
spring-list-reordering Configuration
spring-masonry Configuration
spring-animating-auto Configuration
spring-multistage-transition Configuration
spring-chain Configuration
spring-svg-filter Configuration
spring-css-keyframes Configuration
pmndrs/react-spring Configuration
pmndrs/react-spring Configuration
initial-rocket Configuration
spring-notification-hub Configuration
spring-notification-hub Configuration
boring-platform-kki52 PR

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@srubin srubin marked this pull request as ready for review December 15, 2021 16:54

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

Just a few questions for my own understanding. But overall it looks great!

@@ -201,7 +212,13 @@ function eachSafely<T>(values: Eachable<T>, each: (value: T) => void) {
/** Tree-shakable state for testing purposes */
export const __raf = {
/** The number of pending tasks */
count: 0,
count(): number {
Copy link
Member

Choose a reason for hiding this comment

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

It'd be helpful for me to understand why we've moved count to a pseudo getter function with a global variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding from the comments is that __raf is intended to be a container for data/functions that are used only in the tests. As of this PR, the count variable is now needed for actual control flow rather than just for test observability, so I moved it to a global.

@joshuaellis joshuaellis changed the base branch from master to release/9.4.0 December 16, 2021 08:35
Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

Will release this in the upcoming 9.4.0 (it'll be released on beta later today). Thank you again!

@joshuaellis joshuaellis merged commit 3be8ca5 into pmndrs:release/9.4.0 Dec 16, 2021
@joshuaellis joshuaellis linked an issue Dec 16, 2021 that may be closed by this pull request
@srubin srubin deleted the sr/stop-raf branch December 16, 2021 15:23
@srubin
Copy link
Contributor Author

srubin commented Dec 16, 2021

No problem, and thank you for the quick merge!

cameron-martin pushed a commit to cameron-martin/react-spring that referenced this pull request May 10, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
* fix(radar): improve radar's legend data to be customizable

* fix(radar): add unit test case and stories

* fix(rader): fix bounded legend data generate bug and apply memoizing

* fix(rader): fix bounded legend data to have a color default value
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 this pull request may close these issues.

AnimationFrame load CPU after component unmounted
5 participants