bernii / gauge.js

100% native and cool looking JavaScript gauge
MIT License
1.42k stars 391 forks source link

Page crash when leaving a tab open with 10 gauges #143

Open paulsc opened 7 years ago

paulsc commented 7 years ago

Hi all,

I've been using gauge.js in one of my projects and it's been very useful. I have an issue where the browser tab running my project will crash when left open for a couple of hours. I think I'm getting close to finding the root cause, but hopefully laying out my thoughts here will help.

I narrowed it down to an issue within gauge.js since I'm getting "[Violation] long running task" messages from Chrome, referring to the AnimationUpdater.run() that's being scheduled with the browser's requestAnimationFrame().

I digged into the code a little bit, and I'm wondering if this code could cause trouble with multiple gauges on the page and a high update frequency. I have 10 gauges that get updated about 10 times per second.

I see for instance that AnimationUpdater.run() gets called on every Gauge.set() call. Every time it runs, it will check all gauges on the page to see if a further animation step is required, and if any of them require additional animation, it will re-schedule itself with requestAnimationFrame().

The problem I see here is that if I call set() on 10 gauges at once, this code will re-schedule itself 10 times instead of just once. Now when these 10 scheduled callbacks kick in, if all gauges still need rendering which is likely, an additional 10 calls will get scheduled. This goes on to a very high number of scheduled tasks.

I think this might be the reason for my crash. According to this requestAnimationFrame() rate can be lowered in background tabs or to save CPU. In this case, my set() calls could arrive at a higher frequency than the AnimationUpdater.run() calls can get scheduled, which would presumably end up scheduling a bunch more calls.

The other thing that leads me to think there could be a problem is the fact that only one "animId" is being tracked at any point in time, which makes it impossible to cancel unrequired outstanding animations if there's more that one scheduled.

If anybody that has experience with this library could confirm/deny I'm going in the right direction that would be great.

Thanks

Paul

kplindegaard commented 7 years ago

@paulsc there could be something here. I haven't really had to dig deep into these things since the gauges in my project update at a fairly low frequency.

However, I think you are correct. The .set() calls may spawn a huge number of callbacks from requestAnimationFrame and build up its callback queue. The logic should be rewritten to prevent that.

kplindegaard commented 7 years ago

Hello again @paulsc. I've tried to implement a mechanism to prevent callbacks from piling up, for example if you do mulitiple calls to set() in between each time the canvases are refreshed. Should also work when the browser page is hidden and not redrawn.

I haven't made a pull request yet, but I would be very interested in you testing the potential fix for me first. Could you do that, please? Download gauge.js from this page and give it a go.

paulsc commented 7 years ago

Hi there!

I won't be able to test it right now since it takes a little time to let the page run, hopefully I'll get around to it soon.

One thing I would note is that I did see requestAnimationFrame() are completely paused if you switch to another tab, so switching to another tab and coming back is a good way of checking if calls are getting queued up.

If you can see only one requestAnimationFrame() call being scheduled at a time, that should definitely fix it, I observed this when I did a similar fix myself which consists of 2 things;

Cheers,

Paul

DmacMcgreg commented 7 years ago

In addition to this, I have noticed that the gauges animate in a very slow and painful manner on mobile devices. I'm wondering if this will help fix the issue as well. You can simulate the behavior by changing the CPU slowdown to x5 in Chrome's Performance tab.

kplindegaard commented 7 years ago

@paulsc I think I'll just merge my fix before I forget all about it but not release a new version right away. @DmacMcgreg Probably not if there is only one gauge. More than one? Hopefully, because the fix won't requestAnimationFrame() as brutally as before.

Knight2601 commented 6 years ago

the simplest option if you're running a lot of gauges within a tab which is all being displayed at once - is to turn off animation or turn the anim speed right up. On mobile devices, i wouldn't recommend animation as every cpu load means battery drain and no one wants their website/app draining too much battery or hitting too high a load on the cpu & memory.

Knight2601 commented 6 years ago

Can someone confirm if this is still an issue for them? Otherwise i'll close this...

Personally, on one of my sites - i'm using up to 25 gauges in a single page application, of which between 2-8 are on screen at a time (scroll) and have left it open for 24 hours without problems...

paulsc commented 6 years ago

I think in the end I patched it by pulling out the animation update function and setting it up to run once every frame for all gauges.

If you add a breakpoint to that function I think you should be able to see it runs more often than required.

Frame rate is a pretty important factor. I think I was running them all (8) at a pretty high frame rate (30/s?) and that made a hit on the CPU.

Cheers,

Paul

paulsc commented 6 years ago

Just re-read the thread. I haven’t been able to test your fix yet.

kplindegaard commented 6 years ago

It's been a while, but I think the first problem @paulsc described on July 26 should at least be reduced now, i.e. avoid that AnimationUpdater() reschedules itself to death. The second one, each .set() can trigger redraw of all gauges, I haven't looked at. It's different. More of a design issue rather than a "bug".

lboes commented 6 years ago

as a workaround, I entered these lines before recreating the meters again, which solved my problem:

    window.AnimationUpdater.elements = [];
    window.AnimationUpdater.animId = null;