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

Seeking very slow with many nodes and keyframes #472

Open
jide opened this issue Jul 5, 2016 · 9 comments
Open

Seeking very slow with many nodes and keyframes #472

jide opened this issue Jul 5, 2016 · 9 comments

Comments

@jide
Copy link

jide commented Jul 5, 2016

I made a little demo of an animation that targets 100 nodes with a SequenceEffect of 5 KeyframeEffect each.

http://jsbin.com/gicoju/edit?js

On chrome, although the animation creation takes time, seeking is ultra fast.
On polyfilled browsers (i.e. safari desktop) the main thread seems unresponsive.

Is this due to the way the polyfill is done, or can this be due to a hidden bottleneck somewhere ?

@alancutter
Copy link
Contributor

I suspect this is due to 409df4d which forces effects to be reapplied as soon as they happen as a work around other browsers not executing RAF in the correct order with regular script.

We should rethink the approach. Perhaps performing the invalidation in a setTimeout(..., 0) instead of immediately. @dstockwell WDYT?

@alancutter
Copy link
Contributor

Spoke with dstockwell, sounds like setTimeout(..., 0) won't work as the bug fix was for single frame flashes rather than persistant incorrect states. Synchronous updating is the only way to guarantee effects are being applied before frames get rendered.

An alternative solution to improve performance would be to reduce the number of updated animations by adding dirty flags.

@jide
Copy link
Author

jide commented Aug 1, 2016

Thanks for the explanation. I'll keep an eye on the issue, I'm not familiar enough with the internals of the project to suggest something. I guess setImmediate won't help either.

@alancutter
Copy link
Contributor

http://jsbin.com/jinexesado/edit?output

Performance is better but still not as good as it could be. The synchronous update to animation effects to work around browser bugs is what's holding us back.
I also noticed that GC was the dominant use of CPU during loading, being more conservative with our object allocation would help performance there.

@jide
Copy link
Author

jide commented Aug 3, 2016

Yay, it's a great improvement !

I was thinking: In this example, in the end, there is only one running animation at a time, all others have either not started or are already ended (before delay, or after delay + duration)

Couldn't something smart be done to ignore updating animations that are in these inactive states ?

@alancutter
Copy link
Contributor

I suspect we may be redundantly clearing their effects, if so that's something that could be optimised out.

@jide
Copy link
Author

jide commented Aug 4, 2016

For the record, in a react app I did, I kind of did this manually:

I have a lot of animated divs animated one after each other, like in the example. I mount divs around the current time when it changes and pass them the currentTime of the animation. It is much much faster than animating all the divs at once.

What's interesting is that I tried an approach with all the divs empty along with their animations, and then putting things inside on seeking, or mounting the divs and then create the animation on seeking and set the currentTime, and the latter is much more performant.

@jide
Copy link
Author

jide commented Aug 4, 2016

To illustrate what I mean :

Approach 1, performance is bad :

<div>Some content #1</div> // starts at 0
<div/> // starts at 1
<div/> // starts at 2
<div/> // starts at 3
<div/> // starts at 4

// set currentTime on all these divs to 1.5. We fill the divs with their content as currentTime changes :

<div/> // set currentTime 1.5
<div>Some content #2</div> // set currentTime 1.5
<div/> // set currentTime 1.5
<div/> // set currentTime 1.5
<div/> // set currentTime 1.5

Approach 2, performance is better :

<div>Some content #1</div>
<!-- empty -->
<!-- empty -->
<!-- empty -->
<!-- empty -->

// set currentTime to 1.5, mount divs that are in range as currentTime changes and set their currentTime :

<!-- empty -->
<div>Some content #2</div> // set currentTime 1.5
<!-- empty -->
<!-- empty -->
<!-- empty -->

So my conclusion is that setting currentTime on divs that are out of bounds regarding delay and duration takes time, although I'd thought it should not.

On Chrome approach 1 is faster, in polyfilled browsers approach 2 is faster.

@jide
Copy link
Author

jide commented Aug 4, 2016

Said differently: the DOM operation of creating a dom node is faster than setting currentTime on a node that is out of its active duration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants