Reduce memory and CPU resources on offline interstitial while idle.

Reduce memory and CPU resources on offline interstitial while idle.

Previously the blink timer was running continuously using ~5% CPU whilst the
game is not activated in a focused tab.

+ Add blink rate limit to 3 blinks.
+ Kill animation timer when game is not activated.

This should mean a maximum of 21 seconds when the timer is running. After
that the CPU usage drops to <1%. There are some event listeners still running
to listen for when the easter egg is activated.

BUG=633426
Committed: https://crrev.com/6417dc0d755a25243a88aa8d1a5c0cee19b9747e
Cr-Commit-Position: refs/heads/master@{#413879}

[edwardjung, 2016-08-15 16:53:07]

Description was changed from

==========
Reduce memory and CPU resources on offline intersitial while idle.
+ Add blink rate limit.
+ Kill animation timer when game is not activated.

BUG=633426
==========

to

==========
Reduce memory and CPU resources on offline interstitial while idle.

Previously the blink timer was running continuously using ~5% CPU whilst the
game is not activated in a focused tab. 

+ Add blink rate limit to 3 blinks.
+ Kill animation timer when game is not activated.

This should mean a maximum of 21 seconds when the timer is running. After that
the CPU usage drops to <1%. There are some event listeners still running to
listen for easter egg activate. 

BUG=633426
==========

[edwardjung, 2016-08-15 18:00:32]

Description was changed from

==========
Reduce memory and CPU resources on offline interstitial while idle.

Previously the blink timer was running continuously using ~5% CPU whilst the
game is not activated in a focused tab. 

+ Add blink rate limit to 3 blinks.
+ Kill animation timer when game is not activated.

This should mean a maximum of 21 seconds when the timer is running. After that
the CPU usage drops to <1%. There are some event listeners still running to
listen for easter egg activate. 

BUG=633426
==========

to

==========
Reduce memory and CPU resources on offline interstitial while idle.

Previously the blink timer was running continuously using ~5% CPU whilst the
game is not activated in a focused tab. 

+ Add blink rate limit to 3 blinks.
+ Kill animation timer when game is not activated.

This should mean a maximum of 21 seconds when the timer is running. After
that the CPU usage drops to <1%. There are some event listeners still running 
to listen for when the easter egg is activated. 

BUG=633426
==========

[edwardjung, 2016-08-15 18:03:22]

edwardjung@chromium.org changed reviewers:
+ mmenke@chromium.org

[edwardjung, 2016-08-15 18:03:23]

mmenke PTAL. This should resolve the excess CPU consumption.

Thanks.

[mmenke, 2016-08-15 19:23:32]

Is there some reason we have to constantly call update() every frame while he's
just blinking?  Seems like we just need to redraw when he's actually
blinking/unblinking.

https://codereview.chromium.org/2243293002/diff/40001/components/neterror/res...
File components/neterror/resources/offline.js (right):

https://codereview.chromium.org/2243293002/diff/40001/components/neterror/res...
components/neterror/resources/offline.js:47: this.activated = false;
Can you document this value?  It's not clear what it actually means.

https://codereview.chromium.org/2243293002/diff/40001/components/neterror/res...
components/neterror/resources/offline.js:514: * Update the game frame.
+"And schedules next frame update"  (I'm asking for the addition because it took
me quite a while to figure out how the next update call was getting scheduled).

Actually...could you rename this draw, too?  The names "drawPending" and
"update" should match.

As someone who is learning how things work here for the first time, I'm giving
clean up suggestions based on what took me a while to figure out.

https://codereview.chromium.org/2243293002/diff/40001/components/neterror/res...
components/neterror/resources/offline.js:594: this.raq();
Would you mind renaming this method?  "raq" does not say to me "If this method
is called, continues animating".  Maybe scheduleNextUpdate?

https://codereview.chromium.org/2243293002/diff/40001/components/neterror/res...
components/neterror/resources/offline.js:671: this.update();
Think this is worth a comment.  Or could call this.raq instead.

https://codereview.chromium.org/2243293002/diff/40001/components/neterror/res...
components/neterror/resources/offline.js:674: }
Should we return early here, or is the game deliberately started with a jump
this first time only?  (i.e., not after "crashing"?)

https://codereview.chromium.org/2243293002/diff/40001/components/neterror/res...
components/neterror/resources/offline.js:1607: this.groundYPos =
Runner.defaultDimensions.HEIGHT - this.config.HEIGHT -
So does the first blink happen instantly now?

[mmenke, 2016-08-15 19:52:27]

https://codereview.chromium.org/2243293002/diff/40001/components/neterror/res...
File components/neterror/resources/offline.js (right):

https://codereview.chromium.org/2243293002/diff/40001/components/neterror/res...
components/neterror/resources/offline.js:594: this.raq();
On 2016/08/15 19:23:32, mmenke wrote:
> Would you mind renaming this method?  "raq" does not say to me "If this method
> is called, continues animating".  Maybe scheduleNextUpdate?

Or if you rename update to draw, scheduleNextDraw

[edwardjung, 2016-08-16 21:06:20]

On 2016/08/15 19:23:32, mmenke wrote:
> Is there some reason we have to constantly call update() every frame while
he's
> just blinking?  Seems like we just need to redraw when he's actually
> blinking/unblinking.

The update method checks so it doesn't do any redrawing unless there's a change
in animation frame. During the delay no redrawing happens. 

It's possible to set a separate delay timer for just the blink delay to avoid
calling update but the performance gains are minimal (still running a timer) for
the few seconds that there isn't any blinking. After the three blinks no more
update calls are made until the game is started. 

requestAnimationFrame also pauses itself if you unfocus the tab, which normal
timers don't. So if you reload a bunch of tabs whilst offline they won't all be
running the blink delay timers in the background.

https://codereview.chromium.org/2243293002/diff/40001/components/neterror/res...
File components/neterror/resources/offline.js (right):

https://codereview.chromium.org/2243293002/diff/40001/components/neterror/res...
components/neterror/resources/offline.js:47: this.activated = false;
On 2016/08/15 19:23:32, mmenke wrote:
> Can you document this value?  It's not clear what it actually means.

Reading through I can see how this is confusing, so I've renamed the activate to
playing and started to activated.

https://codereview.chromium.org/2243293002/diff/40001/components/neterror/res...
components/neterror/resources/offline.js:514: * Update the game frame.
On 2016/08/15 19:23:32, mmenke wrote:
> +"And schedules next frame update"  (I'm asking for the addition because it
took
> me quite a while to figure out how the next update call was getting
scheduled).
> 
> Actually...could you rename this draw, too?  The names "drawPending" and
> "update" should match.
> 
> As someone who is learning how things work here for the first time, I'm giving
> clean up suggestions based on what took me a while to figure out.

Sure thing, appreciate the input. Added the comment, this method does more than
draw, as it has the update logic for playing sounds, calling collision
detection, detecting night mode and more. 

I updated drawPending to updatePending to make it consistent.

https://codereview.chromium.org/2243293002/diff/40001/components/neterror/res...
components/neterror/resources/offline.js:594: this.raq();
On 2016/08/15 19:52:27, mmenke wrote:
> On 2016/08/15 19:23:32, mmenke wrote:
> > Would you mind renaming this method?  "raq" does not say to me "If this
method
> > is called, continues animating".  Maybe scheduleNextUpdate?
> 
> Or if you rename update to draw, scheduleNextDraw

Updated. It's shorthand for requestAnimationFrame Animation Queue but guess that
pretty obscure for most people not using requestAnimationFrame.

https://codereview.chromium.org/2243293002/diff/40001/components/neterror/res...
components/neterror/resources/offline.js:671: this.update();
On 2016/08/15 19:23:32, mmenke wrote:
> Think this is worth a comment.  Or could call this.raq instead.

Done.

https://codereview.chromium.org/2243293002/diff/40001/components/neterror/res...
components/neterror/resources/offline.js:674: }
On 2016/08/15 19:23:32, mmenke wrote:
> Should we return early here, or is the game deliberately started with a jump
> this first time only?  (i.e., not after "crashing"?)

This is deliberate, we start on a jump and sound effect. Added a comment.

https://codereview.chromium.org/2243293002/diff/40001/components/neterror/res...
components/neterror/resources/offline.js:1607: this.groundYPos =
Runner.defaultDimensions.HEIGHT - this.config.HEIGHT -
On 2016/08/15 19:23:32, mmenke wrote:
> So does the first blink happen instantly now?

No, this was a duplicate as the blink delay is already being set in the update
method when I set the state to waiting.

[mmenke, 2016-08-16 21:22:35]

On 2016/08/16 21:06:20, edwardjung wrote:
> On 2016/08/15 19:23:32, mmenke wrote:
> > Is there some reason we have to constantly call update() every frame while
> he's
> > just blinking?  Seems like we just need to redraw when he's actually
> > blinking/unblinking.
> 
> The update method checks so it doesn't do any redrawing unless there's a
change
> in animation frame. During the delay no redrawing happens. 
> 
> It's possible to set a separate delay timer for just the blink delay to avoid
> calling update but the performance gains are minimal (still running a timer)
for
> the few seconds that there isn't any blinking. After the three blinks no more
> update calls are made until the game is started. 
> 
> requestAnimationFrame also pauses itself if you unfocus the tab, which normal
> timers don't. So if you reload a bunch of tabs whilst offline they won't all
be
> running the blink delay timers in the background.

The confuses me - if we're doing so little work most of the time, and only
redrawing once every couple of seconds, why are we using 5% CPU time?

[edwardjung, 2016-08-17 01:05:41]

On 2016/08/16 21:22:35, mmenke wrote:
> On 2016/08/16 21:06:20, edwardjung wrote:
> > On 2016/08/15 19:23:32, mmenke wrote:
> > > Is there some reason we have to constantly call update() every frame while
> > he's
> > > just blinking?  Seems like we just need to redraw when he's actually
> > > blinking/unblinking.
> > 
> > The update method checks so it doesn't do any redrawing unless there's a
> change
> > in animation frame. During the delay no redrawing happens. 
> > 
> > It's possible to set a separate delay timer for just the blink delay to
avoid
> > calling update but the performance gains are minimal (still running a timer)
> for
> > the few seconds that there isn't any blinking. After the three blinks no
more
> > update calls are made until the game is started. 
> > 
> > requestAnimationFrame also pauses itself if you unfocus the tab, which
normal
> > timers don't. So if you reload a bunch of tabs whilst offline they won't all
> be
> > running the blink delay timers in the background.
> 
> The confuses me - if we're doing so little work most of the time, and only
> redrawing once every couple of seconds, why are we using 5% CPU time?

I'll need to recheck the CPU usage after the last set of changes and do some
more testing (I did test a version switching to setTimeout) when I'm back from
being OOO.

[edwardjung, 2016-08-23 12:41:33]

> The confuses me - if we're doing so little work most of the time, and only
> redrawing once every couple of seconds, why are we using 5% CPU time?

Okay, I had another chance to look at this again today.

CPU usage (offline dino / test page with just a RequestAnimationFrame call
running / test page with SetInterval timer):
- Windows (0.1% / 0.1% / 0.5%)
- Mac  (4-5% / ~4% / <1%)
- Linux (~4-6% / ~4-6% / <1%)

The RAF timer seems to take up most of the CPU clock time aiming for 60fps and
calls a function 60 times a second since this is for animations. SetInterval on
the other hand uses >1% CPU as it doesn't make any function calls except at the
end of the timer when CPU rises.  Curious that on Windows, RAF is very efficient
but on Mac and Linux it's not, I don't have another WIndows machine to compare
this on, but all three machines are more powerful than standard consumer models.


The main advantage of RAF is that it's automatically stops running if the tab is
in the background whereas SetInterval keeps on running. So if loading a number
of tabs post crash or relaunch and you're offline RAF should be more efficient. 
With the blink occurring once every 7 seconds the maximum time there would be
CPU usage would be 21 seconds, but almost always less. 

I'm not sure of the marginal efficiency gain here from the switch to
SetInterval, since I'll have to add extra listeners to check whether the tab is
focused or not to pause the timer. What do you think?  If you think it's worth
doing, I would like to tackle that refactoring in a separate CL. Limiting the
blink rate itself will vastly reduce CPU usage.

[mmenke, 2016-08-23 14:38:45]

On 2016/08/23 12:41:33, edwardjung wrote:
> > The confuses me - if we're doing so little work most of the time, and only
> > redrawing once every couple of seconds, why are we using 5% CPU time?
> 
> Okay, I had another chance to look at this again today.
> 
> CPU usage (offline dino / test page with just a RequestAnimationFrame call
> running / test page with SetInterval timer):
> - Windows (0.1% / 0.1% / 0.5%)
> - Mac  (4-5% / ~4% / <1%)
> - Linux (~4-6% / ~4-6% / <1%)
> 
> The RAF timer seems to take up most of the CPU clock time aiming for 60fps and
> calls a function 60 times a second since this is for animations. SetInterval
on
> the other hand uses >1% CPU as it doesn't make any function calls except at
the
> end of the timer when CPU rises.  Curious that on Windows, RAF is very
efficient
> but on Mac and Linux it's not, I don't have another WIndows machine to compare
> this on, but all three machines are more powerful than standard consumer
models.
> 
> 
> The main advantage of RAF is that it's automatically stops running if the tab
is
> in the background whereas SetInterval keeps on running. So if loading a number
> of tabs post crash or relaunch and you're offline RAF should be more
efficient. 
> With the blink occurring once every 7 seconds the maximum time there would be
> CPU usage would be 21 seconds, but almost always less. 
> 
> I'm not sure of the marginal efficiency gain here from the switch to
> SetInterval, since I'll have to add extra listeners to check whether the tab
is
> focused or not to pause the timer. What do you think?  If you think it's worth
> doing, I would like to tackle that refactoring in a separate CL. Limiting the
> blink rate itself will vastly reduce CPU usage.

I think this is fine for now, LGTM.  Does seem unfortunate that
RequestAnimationFrame consumes so much CPU time, though.  Could file a bug about
that, and see what the response is.

https://codereview.chromium.org/2243293002/diff/80001/components/neterror/res...
File components/neterror/resources/offline.js (right):

https://codereview.chromium.org/2243293002/diff/80001/components/neterror/res...
components/neterror/resources/offline.js:47: this.playing = false; // Whether
the game is currently in play state.
Thanks!  This makes the code much easier to follow.

[edwardjung, 2016-08-23 16:13:55]

> I think this is fine for now, LGTM.  Does seem unfortunate that
> RequestAnimationFrame consumes so much CPU time, though.  Could file a bug
about
> that, and see what the response is.

I can see a few bugs out there related to RAF and excessive CPU usage on OSX,
seems like RAF does layout updates even when not required:
https://bugs.chromium.org/p/chromium/issues/detail?id=476158
https://bugs.chromium.org/p/chromium/issues/detail?id=505056

Had one extra change as spotted a bug when activating the game whilst it was
idle post blinking stage.

https://codereview.chromium.org/2243293002/diff/100001/components/neterror/re...
File components/neterror/resources/offline.js (right):

https://codereview.chromium.org/2243293002/diff/100001/components/neterror/re...
components/neterror/resources/offline.js:671: this.update();
Scheduling the update before updating the game frame causes the initial jump to
be janky missing a few frames so switching this back to call update.

[mmenke, 2016-08-23 17:30:04]

On 2016/08/23 16:13:55, edwardjung wrote:
> > I think this is fine for now, LGTM.  Does seem unfortunate that
> > RequestAnimationFrame consumes so much CPU time, though.  Could file a bug
> about
> > that, and see what the response is.
> 
> I can see a few bugs out there related to RAF and excessive CPU usage on OSX,
> seems like RAF does layout updates even when not required:
> https://bugs.chromium.org/p/chromium/issues/detail?id=476158
> https://bugs.chromium.org/p/chromium/issues/detail?id=505056
> 
> Had one extra change as spotted a bug when activating the game whilst it was
> idle post blinking stage.
> 
>
https://codereview.chromium.org/2243293002/diff/100001/components/neterror/re...
> File components/neterror/resources/offline.js (right):
> 
>
https://codereview.chromium.org/2243293002/diff/100001/components/neterror/re...
> components/neterror/resources/offline.js:671: this.update();
> Scheduling the update before updating the game frame causes the initial jump
to
> be janky missing a few frames so switching this back to call update.

Still LGTM

[edwardjung, 2016-08-23 18:00:48]

The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 18:01:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 19:11:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 19:11:09]

Dry run: This issue passed the CQ dry run.

[edwardjung, 2016-08-23 22:49:25]

The CQ bit was checked by edwardjung@chromium.org

[commit-bot: I haz the power, 2016-08-23 22:50:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 22:56:08]

Description was changed from

==========
Reduce memory and CPU resources on offline interstitial while idle.

Previously the blink timer was running continuously using ~5% CPU whilst the
game is not activated in a focused tab. 

+ Add blink rate limit to 3 blinks.
+ Kill animation timer when game is not activated.

This should mean a maximum of 21 seconds when the timer is running. After
that the CPU usage drops to <1%. There are some event listeners still running 
to listen for when the easter egg is activated. 

BUG=633426
==========

to

==========
Reduce memory and CPU resources on offline interstitial while idle.

Previously the blink timer was running continuously using ~5% CPU whilst the
game is not activated in a focused tab. 

+ Add blink rate limit to 3 blinks.
+ Kill animation timer when game is not activated.

This should mean a maximum of 21 seconds when the timer is running. After
that the CPU usage drops to <1%. There are some event listeners still running 
to listen for when the easter egg is activated. 

BUG=633426
==========

[commit-bot: I haz the power, 2016-08-23 22:56:10]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-23 22:57:40]

Description was changed from

==========
Reduce memory and CPU resources on offline interstitial while idle.

Previously the blink timer was running continuously using ~5% CPU whilst the
game is not activated in a focused tab. 

+ Add blink rate limit to 3 blinks.
+ Kill animation timer when game is not activated.

This should mean a maximum of 21 seconds when the timer is running. After
that the CPU usage drops to <1%. There are some event listeners still running 
to listen for when the easter egg is activated. 

BUG=633426
==========

to

==========
Reduce memory and CPU resources on offline interstitial while idle.

Previously the blink timer was running continuously using ~5% CPU whilst the
game is not activated in a focused tab.

+ Add blink rate limit to 3 blinks.
+ Kill animation timer when game is not activated.

This should mean a maximum of 21 seconds when the timer is running. After
that the CPU usage drops to <1%. There are some event listeners still running
to listen for when the easter egg is activated.

BUG=633426

Committed: https://crrev.com/6417dc0d755a25243a88aa8d1a5c0cee19b9747e
Cr-Commit-Position: refs/heads/master@{#413879}
==========

[commit-bot: I haz the power, 2016-08-23 22:57:41]

Patchset 6 (id:??) landed as
https://crrev.com/6417dc0d755a25243a88aa8d1a5c0cee19b9747e
Cr-Commit-Position: refs/heads/master@{#413879}