Set IdleHandler timer on background threads a minimum of 30s in the future after initial run.

Set IdleHandler timer on background threads a minimum of 30s in the future after initial run.

BUG=394187
Committed: https://crrev.com/cd45a3ee8f4ebb2adf7daf383a83267f8dbe5f3b
Cr-Commit-Position: refs/heads/master@{#295705}

[sullivan, 2014-09-18 18:56:21]

sullivan@chromium.org changed reviewers:
+ jochen@chromium.org

[sullivan, 2014-09-18 18:56:22]

I've sent out some perf tryjobs for energy cases, are there other perf tests I
should run tryjobs for?
https://codereview.chromium.org/584473005
https://codereview.chromium.org/580203003

[jochen (gone - plz use gerrit), 2014-09-18 19:00:56]

we don't have a good test for this

if it helps with battery live, lgtm

[sullivan, 2014-09-18 19:06:34]

On 2014/09/18 19:00:56, jochen wrote:
> we don't have a good test for this

What level of test would be best to see? A unit test for
RenderThreadImpl::IdleHandler?

> if it helps with battery live, lgtm

Will wait to submit until I have numbers

[jochen (gone - plz use gerrit), 2014-09-19 13:28:00]

On 2014/09/18 at 19:06:34, sullivan wrote:
> On 2014/09/18 19:00:56, jochen wrote:
> > we don't have a good test for this
> 
> What level of test would be best to see? A unit test for
RenderThreadImpl::IdleHandler?

i think that's overkill for this CL, but eventually, we should have some perf
test that makes sure that a background tab uses as little memory as possible.

> > if it helps with battery live, lgtm
> 
> Will wait to submit until I have numbers

[sullivan, 2014-09-19 13:39:08]

On 2014/09/19 13:28:00, jochen (slow for reviews) wrote:
> On 2014/09/18 at 19:06:34, sullivan wrote:
> > On 2014/09/18 19:00:56, jochen wrote:
> > > we don't have a good test for this
> > 
> > What level of test would be best to see? A unit test for
> RenderThreadImpl::IdleHandler?
> 
> i think that's overkill for this CL, but eventually, we should have some perf
> test that makes sure that a background tab uses as little memory as possible.

Yeah, I would love to work on improving our memory tests overall, once we've got
battery life a little more under control.

> > > if it helps with battery live, lgtm
> > 
> > Will wait to submit until I have numbers

So the perf trybots are really overloaded, and a bunch of bugs makes it so their
energy numbers aren't great. I talked to Tony, and we're going to submit this
and revert if any issues show up on chromium.perf.

[sullivan, 2014-09-19 13:39:11]

The CQ bit was checked by sullivan@chromium.org

[commit-bot: I haz the power, 2014-09-19 13:40:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/580173003/1

[commit-bot: I haz the power, 2014-09-19 14:39:25]

Committed patchset #1 (id:1) as 340fa0241a8d998b75668a2c6fdfc676fc134068

[commit-bot: I haz the power, 2014-09-19 14:39:59]

Patchset 1 (id:??) landed as
https://crrev.com/cd45a3ee8f4ebb2adf7daf383a83267f8dbe5f3b
Cr-Commit-Position: refs/heads/master@{#295705}

[pasko, 2014-10-09 09:32:14]

pasko@chromium.org changed reviewers:
+ pasko@chromium.org

[pasko, 2014-10-09 09:32:14]

fly by review.. (I am exercising my math)

https://codereview.chromium.org/580173003/diff/1/content/renderer/render_thre...
File content/renderer/render_thread_impl.cc (left):

https://codereview.chromium.org/580173003/diff/1/content/renderer/render_thre...
content/renderer/render_thread_impl.cc:1030: // kInitialIdleHandlerDelayMs in
RenderThreadImpl::WidgetHidden.
why deleting this comment?

https://codereview.chromium.org/580173003/diff/1/content/renderer/render_thre...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/580173003/diff/1/content/renderer/render_thre...
content/renderer/render_thread_impl.cc:1028: //    30s, 30, 30, 31, 31, 31, 31,
32, 32, ...
with the new formula the series is actually growing slower:
(30s) would be repeated >30 times,
followed by (31s) repeated >30 times,
.. and so on

(I am assuming the series start from kInitialIdleHandlerDelayMs)

That's probably aligned with your intentions, but the comment is now more
confusing.

[sullivan, 2014-10-10 20:55:27]

https://codereview.chromium.org/580173003/diff/1/content/renderer/render_thre...
File content/renderer/render_thread_impl.cc (left):

https://codereview.chromium.org/580173003/diff/1/content/renderer/render_thre...
content/renderer/render_thread_impl.cc:1030: // kInitialIdleHandlerDelayMs in
RenderThreadImpl::WidgetHidden.
On 2014/10/09 09:32:14, pasko wrote:
> why deleting this comment?

It was a little misleading, since it's reset to kInitialIdleHandlerDelayMs (1s)
but then below immediately limited to kLongIdleHandlerDelayMs (30s) on the next
run.

So the previous behavior was that the tab is backgrounded and timers would fire
1s, 1s, 1s, 1s, 2s, 2s... and now the behavior is the tab is backgrounded and
timers fire 1s, 30s, 30s, 30s....

I felt that it was clearer to add the comment on line 1020 instead:

When the tab is originally hidden, an invocation
is scheduled for kInitialIdleHandlerDelayMs in
RenderThreadImpl::WidgetHidden in order to race to a minimal heap.
After that, idle calls can be much less frequent, so run at a maximum of
once every kLongIdleHandlerDelayMs.

https://codereview.chromium.org/580173003/diff/1/content/renderer/render_thre...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/580173003/diff/1/content/renderer/render_thre...
content/renderer/render_thread_impl.cc:1028: //    30s, 30, 30, 31, 31, 31, 31,
32, 32, ...
On 2014/10/09 09:32:14, pasko wrote:
> with the new formula the series is actually growing slower:
> (30s) would be repeated >30 times,
> followed by (31s) repeated >30 times,
> .. and so on
> 
> (I am assuming the series start from kInitialIdleHandlerDelayMs)
> 
> That's probably aligned with your intentions, but the comment is now more
> confusing.

I added a new patch to try and clarify the comments, but I'm having difficulty
explaining it clearly. Suggestions welcome!