Skip to content

Possible infinite loop in FrameLoop.ts update method #770

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

Closed
thatsprettyfaroutman opened this issue Jul 31, 2019 · 8 comments
Closed

Possible infinite loop in FrameLoop.ts update method #770

thatsprettyfaroutman opened this issue Jul 31, 2019 · 8 comments
Labels
area: performance issues with performance in particular targets kind: bug Something isn't working

Comments

@thatsprettyfaroutman
Copy link

thatsprettyfaroutman commented Jul 31, 2019

🐛 Bug Report

Possible infinite loop in FrameLoop.ts update method on low end machines 📠

In this part of the code (line 74):

if (time > lastTime + 64) lastTime = time; // http://gafferongames.com/game-physics/fix-your-timestep/

let numSteps = Math.floor(time - lastTime);

What happens is when the delta time between two frames is longer than 64 ms numSteps will become 0 and the animation won't move forward, causing it to get stuck

To Reproduce

Steps to reproduce the behavior:

Start a low end machine on vm where delta time between frames gets longer than 64 ms. Alternatively force the numSteps to always be zero.

Expected behavior

Animations do not get stuck on low frame rate

Link to repro (highly encouraged)

https://codesandbox.io/s/fervent-carson-n4m3p

Environment

  • react-spring v8.0.23
  • react v16.8.6

Fix

Remove the line 74: if (time > lastTime + 64) lastTime = time;

@thatsprettyfaroutman thatsprettyfaroutman added the kind: bug Something isn't working label Jul 31, 2019
@thatsprettyfaroutman
Copy link
Author

Possibly related to #767 and #720

@thatsprettyfaroutman thatsprettyfaroutman changed the title Possible infinite loop in update method Possible infinite loop in FrameLoop.ts update method Aug 1, 2019
@brunolemos
Copy link
Contributor

brunolemos commented Aug 3, 2019

This happens on high end machines as well, at least on Firefox.

I am investigating performance issues on my app (on a macpro) and there is something very wrong going on. You can see that the FrameLoop is calling update a lot of times even though apparently all animations have already finished: (it should show nothing but shows all these pink and yellow things being executed a lot of times per second):

image

This causes everything to be super laggy. (on both v8 and v9-31)

Use Firefox Developer Edition to reproduce not Chrome.
Both dev and production mode.

@brunolemos
Copy link
Contributor

brunolemos commented Aug 3, 2019

I think it has something to do with having lot's of Animated components rendered in the screen, and some Animated views inside other Animated views. If I render only one the issue doesn't happen, but if I render more things it happens.

@brunolemos
Copy link
Contributor

brunolemos commented Aug 4, 2019

I made some investigations.

  1. It seems to be related to requestAnimationFrame. Overriding this with window.requestIdleCallback fixes things on development (but not on production, for some reason).

    import { Globals } from 'react-spring'
    
    Globals.assign({`
      requestAnimationFrame: (window as any).requestIdleCallback,
      cancelAnimationFrame: (window as any).cancelIdleCallback,
    })

This improves things in development as well, took it from the mozilla docs:

    Globals.assign({
      requestAnimationFrame: (cb?: (time: number) => void) => {
        return window.requestAnimationFrame(t => {
          setTimeout(() => {
            if (cb) cb(t)
          }, 0)
        })
      },
    })
  1. When playing with different requestAnimationFrame overrides, I started getting this error: InternalError: Too much recursion. I checked the line and saw that react-spring searches for Animated components in the whole subtree (and also search the animated parents). Dunno if that's related to this issue, but seems to be one of the performance bottlenecks causes, at least on my app.

    image
  2. Here's a screenshot of the performance tab, notice that the painting is expensive on every frame (taking ~100ms instead of <16ms, making the fps drop to ~5):
    image

    Mozilla docs page has some warnings related to style flush and requestAnimationFrame that I think are related to what's happening. The whole article is worth reading: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Performance_best_practices_for_Firefox_fe_engineers#Detecting_and_avoiding_synchronous_style_flushes

    image image
  3. There's this list specifing what forces a layout paint / reflow, it's funny that it says for loops that force layout & change the DOM are the worst, avoid them, I think that's exactly what react-spring does but I still don't know what exactly could be done to improve this.

@brunolemos
Copy link
Contributor

brunolemos commented Aug 4, 2019

I don't know if the problem is an infinite loop, it seems to be more a "each step of the loop is too slow" so the loop takes a looooong time. Or they are two different problems.

@thatsprettyfaroutman
Copy link
Author

thatsprettyfaroutman commented Aug 8, 2019

Ah, I see! I wonder if the jank is needed tho? At least in my case it seems to be causing more problems than it solves.

We use react-spring in our product and support low end ie11 machines, where the problem gets very severe. I have a case where a transition animation between views always takes more than 200ms per frame. This causes the transition animation to always be janked on every frame and the animation never to proceeds and won't be able to finish. Causing it to get stuck in the animation loop.

However if the jank is disabled these machines are still able to at least finish the animation. Frame rate is low, like 5 fps or something, but the animations finish and the app is usable.

@thatsprettyfaroutman
Copy link
Author

Yeah, you're right its not really an infinite loop, but never ending animation loop or something like that

@aleclarson aleclarson added the area: performance issues with performance in particular targets label Aug 16, 2019
@aleclarson
Copy link
Contributor

Fixed in #808

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: performance issues with performance in particular targets kind: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants