Open andrewreedy opened 9 years ago
@DnMllr Do you think this is due the fact that we're sending empty messages via postMessage
?
Looking into this.
@michaelobriena Thank you :+1: .. I'm a little curious myself, so I'm playing around in engine code a bit. It seems like with 600 nodes, SizeSystem.update() and TransformSystem.update() will each loop through all 600 nodes every frame checking if they have changed. I wonder if it would be better to have the node message the systems when they have changed rather than checking each one of them constantly?
Just my first thought.
I was thinking that too. The idea was that the traversal is so cheap and since nothing has changed we just do no math. 0.3 also did the full traversal but also did all of the math regardless of changes so I think there is something a little funkier at play.
Running this in the browser has really great performance (essentially totally idle) so later today / this weekend I will try and build with cordova to see if that is the issue.
Just need to remove Meteor from the equation to make sure we can pinpoint the issue.
Okay I removed Meteor from the equation. The new master branch of that repo is just plain Cordova with Famous Engine. Ya Ive seen great performance on desktop too. It's just when it's in a webview for some reason?
Also going to test cordova webview vs the standard UIWebView.
do you know if this is the case for 0.6 or 0.5 famous?
I'm not. I haven't tested it with those versions.
Having issues with cordova, will report back.
ok. I'm using Cordova version 5.0.0 btw.
Random discovery: Size.prototype.fromComponents() seems to add about 10% CPU in this test case.
It looks like Transform.prototype.calculate() (fromNode()) might be the most expensive method.
This makes me a little concerned, can you be a little more specific about what the issue is...
On Thursday, August 6, 2015, Andrew Reedy notifications@github.com wrote:
It looks like fromNode() might be the other heavy hitter.
— Reply to this email directly or view it on GitHub https://github.com/Famous/engine/issues/464#issuecomment-128537044.
@jd-carroll I can not say if this is an issue for all Cordova apps. I can only show that it's an issue with this simple case. The CPU usage increases with the number of nodes so for my iPhone 5s the CPU hits 100% at about 450 empty inanimate nodes.
@jd-carroll I agree, this is concerning to me. The thing I can say is that the runtime of our transform math will always be less expensive than it is in 0.3 so I am assuming there is something else at play.
Can you try testing the same app from Safari browser on mobile to confirm the same result? There should be no difference between the Cordova and Safari experience.
On Sunday, August 9, 2015, michaelobriena notifications@github.com wrote:
@jd-carroll https://github.com/jd-carroll I agree, this is concerning to me. The thing I can say is that the runtime of our transform math will always be less expensive than it is in 0.3 so I am assuming there is something else at play.
— Reply to this email directly or view it on GitHub https://github.com/Famous/engine/issues/464#issuecomment-129234444.
@jd-carroll Well thats not entirely true; UIWebView uses a different JS engine than Safari iOS and I'm not sure if I can do the same XCode CPU profile on a mobile website
The issue has to do with TransformSystem.update. We aren't gating correctly. There are some corner cases we are working through but your example was running at 3% on desktop and 30-40% on mobile safari / cordova.
@michaelobriena yea this is what I was thinking as well. It should only run transform.calculate()
on nodes which have changed instead of all nodes correct? I'm sure it depends on the device you are running it on (as far as % CPU). Either way 40% on an app that isn't doing anything is an issue right?
@andrewreedy correct. That said, this fix doesn't solve everything. We are seeing that even a blank request animation frame is running at around 10% CPU usage on mobile safari.
@michaelobriena What edge cases wouldn be missed if Transform.update did not run every frame?
On Monday, August 10, 2015, michaelobriena notifications@github.com wrote:
@andrewreedy https://github.com/andrewreedy correct. That said, this fix doesn't solve everything. We are seeing that even a blank request animation frame is running at around 10% CPU usage on mobile safari.
— Reply to this email directly or view it on GitHub https://github.com/Famous/engine/issues/464#issuecomment-129657795.
Nothing would move for that frame.
@michaelobriena Were you able to test this using the WKWebView? While switching web views doesn't necessarily solve the problem, it does seem to dramatically help.
@michaelobriena Any updates/progress on this?
I'm curious about this too, as I'm having bad perf in Chrome on Nexus 5, and I can't tell why. The scene graph is not complex, but it's just slow (the framerate).
@andrewreedy can you simulate the process using crosswalk instead? Let me know
@lxndralbert I'm currently only focused on iOS
Are we still experiencing this issue?
I'm starting a project and would like to know if this still persists or if someone has a fix.
Thank you!
My new release of infamous@5.2.0
solves this problem in general and I believe is a better direction to go in: https://github.com/infamous/infamous/releases/tag/v5.2.0
When no updates exist, 0% CPU is used, and no rAF loop exists at all. Up next will be providers (for example something like a TransitionableTransform
from Famous 0.3) that add/remove render tasks to/from Nodes. This will make it easy to, for example, animate a number from one value to the next. When the animation completes, the render task is removed, and the CPU goes back to completely idle.
sounds great!!
I was wondering why my iOS Famous app was using 100% CPU so I made an empty test app to see at what point the CPU spike started. It seems even with empty nodes which aren't animating the CPU spikes pretty quickly. Any thoughts on whats causing this?
Famous version: 0.7.1
Other Note: The app in Famous 0.3.0 used about 5% CPU
Test Repo: https://github.com/andrewreedy/engine-meteor-cordova-test