[content]: Add CanExceedIdleDeadlineIfRequired function to RendererScheduler.

[content]: Add CanExceedIdleDeadlineIfRequired function to RendererScheduler.

Adds an CanExceedIdleDeadlineIfRequired function to RendererScheduler
to enable IdleTasks to estimate the best time for them to exceed their deadline
if they have a large monolithic task which could not be run otherwise.

Long idle task design doc:
https://docs.google.com/a/chromium.org/document/d/1yBlUdYW8VTIfB-DqhvQqUeP0kf-Ap1W4cao2yQq58Do/edit

BUG=455713
Committed: https://crrev.com/bf2ac8943e31203dbb71680d19fadd822f8d7601
Cr-Commit-Position: refs/heads/master@{#320817}

[rmcilroy, 2015-03-03 12:38:23]

rmcilroy@chromium.org changed reviewers:
+ skyostil@chromium.org

[rmcilroy, 2015-03-03 12:38:23]

Sami: This is the chromium side of https://codereview.chromium.org/971283002/ -
PTAL, thanks.

[Sami, 2015-03-03 16:21:33]

https://codereview.chromium.org/969373002/diff/1/content/renderer/scheduler/r...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/969373002/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_impl.cc:512: // Allow one
millisecond slop.
I was thinking about the need to have this back-and-forth between the scheduler
and the client to figure out whether this was the maximum deadline or not and
that the answer depends on when you call this function even if the deadline
itself remains the same.

Should we instead frame this as IsAllowedToExceedDeadline() or something to that
effect to make it less about the deadline itself but more about the underlying
system state? One reason for doing this is that perhaps we want to start giving
out super long deadlines for backgrounded tabs or something, in which case this
function needs to start lying :)

[rmcilroy, 2015-03-03 17:01:36]

On 2015/03/03 16:21:33, Sami wrote:
>
https://codereview.chromium.org/969373002/diff/1/content/renderer/scheduler/r...
> File content/renderer/scheduler/renderer_scheduler_impl.cc (right):
> 
>
https://codereview.chromium.org/969373002/diff/1/content/renderer/scheduler/r...
> content/renderer/scheduler/renderer_scheduler_impl.cc:512: // Allow one
> millisecond slop.
> I was thinking about the need to have this back-and-forth between the
scheduler
> and the client to figure out whether this was the maximum deadline or not and
> that the answer depends on when you call this function even if the deadline
> itself remains the same.
> 
> Should we instead frame this as IsAllowedToExceedDeadline() or something to
that
> effect to make it less about the deadline itself but more about the underlying
> system state?  One reason for doing this is that perhaps we want to start
giving
> out super long deadlines for backgrounded tabs or something, in which case
this
> function needs to start lying :)

Yes, this might be better - I'll rework it to be something like this (maybe with
a slightly different name).

Are we planning on giving out super-long deadlines for background tabs?  Isn't
there the same issue that if someone switches tabs we want to respond to that
within 50ms if we can? Note: I could see that we might want to have super
spaced-out idle periods to save on power / cpu, but I would think the deadlines
should still stay at 50ms.

[Sami, 2015-03-03 18:22:52]

On 2015/03/03 17:01:36, rmcilroy wrote:
> Yes, this might be better - I'll rework it to be something like this (maybe
with
> a slightly different name).

sgtm!

> Are we planning on giving out super-long deadlines for background tabs?  Isn't
> there the same issue that if someone switches tabs we want to respond to that
> within 50ms if we can? Note: I could see that we might want to have super
> spaced-out idle periods to save on power / cpu, but I would think the
deadlines
> should still stay at 50ms.

No, I wasn't seriously proposing that -- it was mainly an example of how we
could be accidentally tying our hands w.r.t. future policy updates. Maybe a more
realistic example would be getting some deadlines fed to us from the kernel's
power management system (e.g., "since this core is already awake at operating
point X, you may as well do Y ms idle work without extra cost").

[picksi, 2015-03-04 10:56:13]

picksi@google.com changed reviewers:
+ picksi@google.com

[picksi, 2015-03-04 10:56:14]

https://codereview.chromium.org/969373002/diff/1/content/renderer/scheduler/r...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/969373002/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_impl.cc:514:
base::TimeDelta::FromMilliseconds(1);
Nit: You could remove the need for the "one millisecond slop" comment if you had
a named const for the '1'.

[rmcilroy, 2015-03-05 11:20:36]

https://codereview.chromium.org/969373002/diff/1/content/renderer/scheduler/r...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/969373002/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_impl.cc:512: // Allow one
millisecond slop.
On 2015/03/03 16:21:32, Sami wrote:
> I was thinking about the need to have this back-and-forth between the
scheduler
> and the client to figure out whether this was the maximum deadline or not and
> that the answer depends on when you call this function even if the deadline
> itself remains the same.
> 
> Should we instead frame this as IsAllowedToExceedDeadline() or something to
that
> effect to make it less about the deadline itself but more about the underlying
> system state? One reason for doing this is that perhaps we want to start
giving
> out super long deadlines for backgrounded tabs or something, in which case
this
> function needs to start lying :)

I've reworked this to expose a CanExceedIdleDeadlineIfRequired, which I think is
a much better solution - thanks for the idea.  PTAL.

https://codereview.chromium.org/969373002/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_impl.cc:514:
base::TimeDelta::FromMilliseconds(1);
On 2015/03/04 10:56:14, picksi wrote:
> Nit: You could remove the need for the "one millisecond slop" comment if you
had
> a named const for the '1'.

Removed all this code :).

[Sami, 2015-03-05 11:59:33]

Thanks, this looks like a better concept to build on.

https://codereview.chromium.org/969373002/diff/20001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/969373002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:476: if
(idle_period_state_ != IdlePeriodState::NOT_WITHIN_IDLE_PERIOD) {
Should we still mark the async event as ended here so EndIdlePeriod() won't try
to do it again?

https://codereview.chromium.org/969373002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:533: return
idle_period_state_ == IdlePeriodState::WITHIN_MAX_DEADLINE_IDLE_PERIOD;
DCHECK(main_thread_checker_.CalledOnValidThread());

https://codereview.chromium.org/969373002/diff/20001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/969373002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.h:166: base::TimeDelta*
next_long_idle_period_delay);
Did you mean to drop the _out here? It's still referenced by the comment.

https://codereview.chromium.org/969373002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.h:195: IdlePeriodState
idle_period_state_;
Not initialized?

[picksi, 2015-03-05 12:22:08]

Yay for Enums!

More "!" comments auto-generated by my "not-bot".

https://codereview.chromium.org/969373002/diff/40001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/969373002/diff/40001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:419: if
(!next_pending_delayed_task.is_null()) {
It's time for my regular "remove ! and swap if/else bodies" comment :)

https://codereview.chromium.org/969373002/diff/40001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:429: return
long_idle_period_duration == max_long_idle_period_duration ?
It seems slightly non-intuitive to say we're "within" the max-deadline-period
when the code is checking for being right at the end (and not a moment before!)
the deadline period. Maybe a comment to explain this apparent contradiction
would help?

https://codereview.chromium.org/969373002/diff/40001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:452: if
(new_idle_period_state != IdlePeriodState::NOT_WITHIN_IDLE_PERIOD) {
(More "!" comments)... I find this double negative (not not within the idle
period) tricky to get my head around (and it appears in a couple of places). It
might be clearer to have a function called
isWithinIdlePeriod(new_idle_period_state) to wrap this.

https://codereview.chromium.org/969373002/diff/40001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:515: if
(idle_period_state_ == IdlePeriodState::NOT_WITHIN_IDLE_PERIOD)
Is this a silent failure? Should this be a DCHECK or is it OK to end a
non-existent idle period?

https://codereview.chromium.org/969373002/diff/40001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:625: IdlePeriodState
state) {
nit: rename state to idle_period_state

[rmcilroy, 2015-03-05 13:25:24]

https://codereview.chromium.org/969373002/diff/40001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/969373002/diff/40001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:419: if
(!next_pending_delayed_task.is_null()) {
On 2015/03/05 12:22:08, picksi wrote:
> It's time for my regular "remove ! and swap if/else bodies" comment :)

Done.

https://codereview.chromium.org/969373002/diff/40001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:429: return
long_idle_period_duration == max_long_idle_period_duration ?
On 2015/03/05 12:22:08, picksi wrote:
> It seems slightly non-intuitive to say we're "within" the max-deadline-period
> when the code is checking for being right at the end (and not a moment
before!)
> the deadline period. Maybe a comment to explain this apparent contradiction
> would help?

I renamed the states (e.g., IN_LONG_IDLE_WITH_MAX_DEADLINE_PERIOD) - does this
make it clearer?

https://codereview.chromium.org/969373002/diff/40001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:452: if
(new_idle_period_state != IdlePeriodState::NOT_WITHIN_IDLE_PERIOD) {
On 2015/03/05 12:22:08, picksi wrote:
> (More "!" comments)... I find this double negative (not not within the idle
> period) tricky to get my head around (and it appears in a couple of places).
It
> might be clearer to have a function called
> isWithinIdlePeriod(new_idle_period_state) to wrap this.

Done.

https://codereview.chromium.org/969373002/diff/40001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:515: if
(idle_period_state_ == IdlePeriodState::NOT_WITHIN_IDLE_PERIOD)
On 2015/03/05 12:22:08, picksi wrote:
> Is this a silent failure? Should this be a DCHECK or is it OK to end a
> non-existent idle period?

No, it's allowable to call EndIdlePeriod even if you aren't within wone
currently - it is just an early-out.  Added a comment.

https://codereview.chromium.org/969373002/diff/40001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:625: IdlePeriodState
state) {
On 2015/03/05 12:22:08, picksi wrote:
> nit: rename state to idle_period_state

Done.

[picksi, 2015-03-05 13:28:09]

Looks good, thanks! one nit: With the newly names enums should
IsWithinIdlePeriod() be called IsInIdlePeriod()?

[rmcilroy, 2015-03-05 13:50:19]

Opps, somehow missed Sami's comments the first time - done now.

On 2015/03/05 13:28:09, picksi wrote:
> Looks good, thanks! one nit: With the newly names enums should
> IsWithinIdlePeriod() be called IsInIdlePeriod()?

Done.

https://codereview.chromium.org/969373002/diff/20001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/969373002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:476: if
(idle_period_state_ != IdlePeriodState::NOT_WITHIN_IDLE_PERIOD) {
On 2015/03/05 11:59:32, Sami wrote:
> Should we still mark the async event as ended here so EndIdlePeriod() won't
try
> to do it again?

No (for the reasons discussed on the other CL).

https://codereview.chromium.org/969373002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:533: return
idle_period_state_ == IdlePeriodState::WITHIN_MAX_DEADLINE_IDLE_PERIOD;
On 2015/03/05 11:59:32, Sami wrote:
> DCHECK(main_thread_checker_.CalledOnValidThread());

Done.

https://codereview.chromium.org/969373002/diff/20001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/969373002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.h:166: base::TimeDelta*
next_long_idle_period_delay);
On 2015/03/05 11:59:33, Sami wrote:
> Did you mean to drop the _out here? It's still referenced by the comment.

opps no, thanks.

https://codereview.chromium.org/969373002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.h:195: IdlePeriodState
idle_period_state_;
On 2015/03/05 11:59:33, Sami wrote:
> Not initialized?

Done.

[Sami, 2015-03-05 14:02:51]

Ack, let's land the other patch first and then rebase this one to avoid
reviewing the same code twice :)

[rmcilroy, 2015-03-05 17:59:15]

On 2015/03/05 14:02:51, Sami wrote:
> Ack, let's land the other patch first and then rebase this one to avoid
> reviewing the same code twice :)

Update rebased against final patch-set of dependent CL - PTAL.

[Sami, 2015-03-05 19:18:07]

Thanks, lgtm with an optional suggestion.

https://codereview.chromium.org/969373002/diff/120001/content/renderer/schedu...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/969373002/diff/120001/content/renderer/schedu...
content/renderer/scheduler/renderer_scheduler_impl.cc:550: return
idle_period_state_ ==
Could you add a trace event here so we can see if we should make this also look
at the current time as a follow-up to avoid overruns?

[rmcilroy, 2015-03-16 22:08:04]

https://codereview.chromium.org/969373002/diff/120001/content/renderer/schedu...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/969373002/diff/120001/content/renderer/schedu...
content/renderer/scheduler/renderer_scheduler_impl.cc:550: return
idle_period_state_ ==
On 2015/03/05 19:18:07, Sami wrote:
> Could you add a trace event here so we can see if we should make this also
look
> at the current time as a follow-up to avoid overruns?

Done.

[rmcilroy, 2015-03-16 22:08:11]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-03-16 22:08:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/969373002/#ps160001
(title: "Add trace event")

[commit-bot: I haz the power, 2015-03-16 22:08:41]

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

[commit-bot: I haz the power, 2015-03-16 23:03:25]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-03-16 23:04:23]

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