[scheduler]: Avoid waking up the scheduler to end long idle periods.

[scheduler]: Avoid waking up the scheduler to end long idle periods.

Reworks the IdleHandler logic to avoid waking the scheduler just to end idle
periods. The new approach avoids posting delayed tasks on the control queue
and instead registers the IdleHandler as a TaskObserver on the message loop,
and checks at the end of each task whether the idle period should end.

Also adds a SingleThreadIdleTaskRunner::Observer, which gets notified when
idle tasks are posted or run. The IdleHelper uses this to decide when to
restart a long-idle period or when to pause long idle period ticks (e.g., if
there are currently no idle tasks queued to be run).

This new model also allow better tracing of idle periods, and the async idle
period tracing code has been improved to use nestable async events, and to only
show "DeadlineOverrun" events if there is actually a idle task running.

Finally adds TRACE_EVENT_NESTABLE_ASYNC_BEGIN/END_WITH_TIMESTAMP0 to
trace_events.h which is required as part of this change.

BUG=485371, 493350
Committed: https://crrev.com/db151ad312913a65a1d4d7d795ca874f644142e0
Cr-Commit-Position: refs/heads/master@{#332414}

[rmcilroy, 2015-05-22 17:01:51]

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

[rmcilroy, 2015-05-22 17:01:52]

This is a reworking of how we end idle periods which avoids the extra wakeups
reported in http://crbug.com/485371.  I've still to fix up the tests and right a
few new ones, but please let me know what you think of the general approach
first.

[rmcilroy, 2015-05-22 17:03:40]

Patchset #1 (id:1) has been deleted

[rmcilroy, 2015-05-26 09:43:16]

+Hannes

Just ran this through v8.top25_smooth and it didn't cause any particular impact
on idle GC times - if anything causing a slight improvement from 47->49%
percentage idle GC (though noisy).

[Sami, 2015-05-26 13:35:41]

Looks like I had a lot of comments but overall I like the simplifying effect
this has.

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
File components/scheduler/child/idle_helper.cc (right):

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:198: // If the idle period has now
been reached, either end the idle period or
"idle period deadline"?

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:215: if
(helper_->IsQueueEmpty(idle_queue_index_)) {
Optimization-ocd-nit: we could combine IsQueueEmpty and QueueNeedsPumped to a
single method that returns a tri-state. Probably not a very big deal either way
can could definitely be done later.

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:232: next_long_idle_period_delay =
idle_period_deadline_ - helper_->Now();
Should we clamp this to zero to avoid random DCHECKs when the deadline is in the
past?

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:248: helper_->CheckOnValidThread();
This can be running on any thread, right?

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:259:
//DCHECK(IsInIdlePeriod(idle_period_state_));
Uncomment/remove?

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:263:
last_traced_start_running_idle_task_ = helper_->Now();
I believe we should use NowFromSystemTraceTime() to make sure everything lines
up on platforms where Now() != NowFromSystemTraceTime() (CrOS). We could
probably just call that directly without going throught the helper. Just make
sure the time stamps are never mixed :)

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:268:
CurrentIdleTaskDeadlineCallback(deadline_out);
Could we just replace this with a simple assigment now? In any case the function
shouldn't be called a "callback" anymore.

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:290: CheckLongIdlePeriodSentinel();
nit: It's not clear what "sentinel" refers to here. Is this essentially
something like UpdateLongIdlePeriodState{AfterIdleTask}()?

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:310: if (new_state ==
idle_period_state_) return;
Should we DCHECK that the deadline didn't change?

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:343:
TRACE_EVENT_NESTABLE_ASYNC_END0(tracing_category_,
Would this work better using the non-nestable STEP_INTO variants? Or do we
actually need nesting in some cases? Maybe I just need to see a trace :)

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:370: nestable_events_started_ =
Weird newline here.

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
File components/scheduler/child/idle_helper.h (right):

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.h:108: //
SingleThreadIdleTaskRunner:Observer implementation:
s/:/::/

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.h:110: void
OnWillRunIdleTask(base::TimeTicks* deadline_out) const override;
Should we make deadline_out a normal return value to emphasize the fact that it
isn't optional?

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
File components/scheduler/child/single_thread_idle_task_runner.h (right):

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/single_thread_idle_task_runner.h:26: // Used request
idle task deadlines and signal posting of idle tasks.
"Used to"?

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/single_thread_idle_task_runner.h:27: class
SCHEDULER_EXPORT Observer {
"Delegate" or "Controller" perhaps since I think computing the deadline goes a
little beyond observing. WDYT?

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/single_thread_idle_task_runner.h:33: virtual void
OnIdleTaskPosted() = 0;
Maybe mention that this will be called on the posting thread?

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/single_thread_idle_task_runner.h:37: virtual void
OnWillRunIdleTask(base::TimeTicks* deadline_out) const = 0;
Could we name these WillProcessIdleTask/DidProcessIdleTask for consistency with
the messageloop observer? In any case, use only "On" or "Will/Did" but not both.

[rmcilroy, 2015-05-27 11:58:37]

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
File components/scheduler/child/idle_helper.cc (right):

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:198: // If the idle period has now
been reached, either end the idle period or
On 2015/05/26 13:35:41, Sami wrote:
> "idle period deadline"?

Done.

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:215: if
(helper_->IsQueueEmpty(idle_queue_index_)) {
On 2015/05/26 13:35:41, Sami wrote:
> Optimization-ocd-nit: we could combine IsQueueEmpty and QueueNeedsPumped to a
> single method that returns a tri-state. Probably not a very big deal either
way
> can could definitely be done later.

Ack. I'll make this change on the preceeding CL and rebase into this CL.

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:232: next_long_idle_period_delay =
idle_period_deadline_ - helper_->Now();
On 2015/05/26 13:35:41, Sami wrote:
> Should we clamp this to zero to avoid random DCHECKs when the deadline is in
the
> past?

Good point! Done.

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:248: helper_->CheckOnValidThread();
On 2015/05/26 13:35:40, Sami wrote:
> This can be running on any thread, right?

Yes, thanks. Done.

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:259:
//DCHECK(IsInIdlePeriod(idle_period_state_));
On 2015/05/26 13:35:41, Sami wrote:
> Uncomment/remove?

Uncommented

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:263:
last_traced_start_running_idle_task_ = helper_->Now();
On 2015/05/26 13:35:40, Sami wrote:
> I believe we should use NowFromSystemTraceTime() to make sure everything lines
> up on platforms where Now() != NowFromSystemTraceTime() (CrOS). We could
> probably just call that directly without going throught the helper. Just make
> sure the time stamps are never mixed :)

Done, although we later pass "std::max(idle_period_deadline_,
last_traced_start_running_idle_task_)" to a trace event and
idle_period_deadline_ is not using NowFromSystemTraceTime(). Should we keep a
separate idle_period_deadline_ initialized via NowFromSystemTraceTime just for
this?

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:268:
CurrentIdleTaskDeadlineCallback(deadline_out);
On 2015/05/26 13:35:41, Sami wrote:
> Could we just replace this with a simple assigment now? In any case the
function
> shouldn't be called a "callback" anymore.

I kept the method for tests. Removed "Callback".

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:290: CheckLongIdlePeriodSentinel();
On 2015/05/26 13:35:40, Sami wrote:
> nit: It's not clear what "sentinel" refers to here. Is this essentially
> something like UpdateLongIdlePeriodState{AfterIdleTask}()?

Yeah, this was originally a sentinal task posted on the idle queue and I never
came up with a better name :).

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:310: if (new_state ==
idle_period_state_) return;
On 2015/05/26 13:35:40, Sami wrote:
> Should we DCHECK that the deadline didn't change?

Done.

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:343:
TRACE_EVENT_NESTABLE_ASYNC_END0(tracing_category_,
On 2015/05/26 13:35:40, Sami wrote:
> Would this work better using the non-nestable STEP_INTO variants? Or do we
> actually need nesting in some cases? Maybe I just need to see a trace :)

Apparently the non-nestable STEP_INTO variants are deprecated and NESTABLE is
the new hotness. It works quite well as long as you keep the nesting consistent
(hence some of the extra bookkeeping requirements. I'll show you a trace.

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:370: nestable_events_started_ =
On 2015/05/26 13:35:40, Sami wrote:
> Weird newline here.

Done.

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
File components/scheduler/child/idle_helper.h (right):

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.h:108: //
SingleThreadIdleTaskRunner:Observer implementation:
On 2015/05/26 13:35:41, Sami wrote:
> s/:/::/

Done.

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.h:110: void
OnWillRunIdleTask(base::TimeTicks* deadline_out) const override;
On 2015/05/26 13:35:41, Sami wrote:
> Should we make deadline_out a normal return value to emphasize the fact that
it
> isn't optional?

Done.

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
File components/scheduler/child/single_thread_idle_task_runner.h (right):

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/single_thread_idle_task_runner.h:26: // Used request
idle task deadlines and signal posting of idle tasks.
On 2015/05/26 13:35:41, Sami wrote:
> "Used to"?

Done.

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/single_thread_idle_task_runner.h:27: class
SCHEDULER_EXPORT Observer {
On 2015/05/26 13:35:41, Sami wrote:
> "Delegate" or "Controller" perhaps since I think computing the deadline goes a
> little beyond observing. WDYT?

Yeah (I originally had Delegate, then moved to Observer when things were simple,
but I agree they are more complex now and Delegate is more appropriate). Done.

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/single_thread_idle_task_runner.h:33: virtual void
OnIdleTaskPosted() = 0;
On 2015/05/26 13:35:41, Sami wrote:
> Maybe mention that this will be called on the posting thread?

Done.

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/single_thread_idle_task_runner.h:37: virtual void
OnWillRunIdleTask(base::TimeTicks* deadline_out) const = 0;
On 2015/05/26 13:35:41, Sami wrote:
> Could we name these WillProcessIdleTask/DidProcessIdleTask for consistency
with
> the messageloop observer? In any case, use only "On" or "Will/Did" but not
both.

Done.

[Sami, 2015-05-27 12:11:56]

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
File components/scheduler/child/idle_helper.cc (right):

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:248: helper_->CheckOnValidThread();
On 2015/05/27 11:58:36, rmcilroy wrote:
> On 2015/05/26 13:35:40, Sami wrote:
> > This can be running on any thread, right?
> 
> Yes, thanks. Done.

Did you mean to add some locking/posting here?-)

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:263:
last_traced_start_running_idle_task_ = helper_->Now();
On 2015/05/27 11:58:36, rmcilroy wrote:
> On 2015/05/26 13:35:40, Sami wrote:
> > I believe we should use NowFromSystemTraceTime() to make sure everything
lines
> > up on platforms where Now() != NowFromSystemTraceTime() (CrOS). We could
> > probably just call that directly without going throught the helper. Just
make
> > sure the time stamps are never mixed :)
> 
> Done, although we later pass "std::max(idle_period_deadline_,
> last_traced_start_running_idle_task_)" to a trace event and
> idle_period_deadline_ is not using NowFromSystemTraceTime(). Should we keep a
> separate idle_period_deadline_ initialized via NowFromSystemTraceTime just for
> this?

I think we should. The two timebases aren't compatible and I believe they will
soon be split into two different time types.

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:343:
TRACE_EVENT_NESTABLE_ASYNC_END0(tracing_category_,
On 2015/05/27 11:58:36, rmcilroy wrote:
> On 2015/05/26 13:35:40, Sami wrote:
> > Would this work better using the non-nestable STEP_INTO variants? Or do we
> > actually need nesting in some cases? Maybe I just need to see a trace :)
> 
> Apparently the non-nestable STEP_INTO variants are deprecated and NESTABLE is
> the new hotness. It works quite well as long as you keep the nesting
consistent
> (hence some of the extra bookkeeping requirements. I'll show you a trace.

Yeah thanks I didn't realize they were deprecated.

https://codereview.chromium.org/1151353003/diff/60001/base/trace_event/trace_...
File base/trace_event/trace_event.h (right):

https://codereview.chromium.org/1151353003/diff/60001/base/trace_event/trace_...
base/trace_event/trace_event.h:764: id, timestamp)        \
Was this git cl format? I wonder if we should be following the existing style
here instead.

https://codereview.chromium.org/1151353003/diff/60001/components/scheduler/ch...
File components/scheduler/child/idle_helper.cc (right):

https://codereview.chromium.org/1151353003/diff/60001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:308: DCHECK(new_deadline ==
idle_period_deadline_);
nit: DCHECK_EQ

[alex clarke (OOO till 29th), 2015-05-27 13:04:16]

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

[alex clarke (OOO till 29th), 2015-05-27 13:04:17]

Drive by comments.  No need to gate on these.

https://codereview.chromium.org/1151353003/diff/60001/components/scheduler/ch...
File components/scheduler/child/idle_helper.cc (right):

https://codereview.chromium.org/1151353003/diff/60001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:127: if (helper_->IsShutdown())
Should we have a test for this?

https://codereview.chromium.org/1151353003/diff/60001/components/scheduler/ch...
File components/scheduler/child/idle_helper.h (right):

https://codereview.chromium.org/1151353003/diff/60001/components/scheduler/ch...
components/scheduler/child/idle_helper.h:156: // Do not modify these field
directly, instead call SetIdlePeriodState.
A random driveby thought.  If we wanted to enforce this (here and elsewhere),
perhaps it's worth defining a struct to hold these with suitable accessors
(SetIdlePeriodState would likely be a member of this struct)?

[rmcilroy, 2015-05-27 19:17:06]

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
File components/scheduler/child/idle_helper.cc (right):

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:215: if
(helper_->IsQueueEmpty(idle_queue_index_)) {
On 2015/05/27 11:58:36, rmcilroy wrote:
> On 2015/05/26 13:35:41, Sami wrote:
> > Optimization-ocd-nit: we could combine IsQueueEmpty and QueueNeedsPumped to
a
> > single method that returns a tri-state. Probably not a very big deal either
> way
> > can could definitely be done later.
> 
> Ack. I'll make this change on the preceeding CL and rebase into this CL.

Done in latest patchset.

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:248: helper_->CheckOnValidThread();
On 2015/05/27 12:11:56, Sami wrote:
> On 2015/05/27 11:58:36, rmcilroy wrote:
> > On 2015/05/26 13:35:40, Sami wrote:
> > > This can be running on any thread, right?
> > 
> > Yes, thanks. Done.
> 
> Did you mean to add some locking/posting here?-)

Opps, added a comment about this then forgot about the rest. Done now.

https://codereview.chromium.org/1151353003/diff/20001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:263:
last_traced_start_running_idle_task_ = helper_->Now();
On 2015/05/27 12:11:56, Sami wrote:
> On 2015/05/27 11:58:36, rmcilroy wrote:
> > On 2015/05/26 13:35:40, Sami wrote:
> > > I believe we should use NowFromSystemTraceTime() to make sure everything
> lines
> > > up on platforms where Now() != NowFromSystemTraceTime() (CrOS). We could
> > > probably just call that directly without going throught the helper. Just
> make
> > > sure the time stamps are never mixed :)
> > 
> > Done, although we later pass "std::max(idle_period_deadline_,
> > last_traced_start_running_idle_task_)" to a trace event and
> > idle_period_deadline_ is not using NowFromSystemTraceTime(). Should we keep
a
> > separate idle_period_deadline_ initialized via NowFromSystemTraceTime just
for
> > this?
> 
> I think we should. The two timebases aren't compatible and I believe they will
> soon be split into two different time types.

Ok, good to know. Done.

https://codereview.chromium.org/1151353003/diff/60001/base/trace_event/trace_...
File base/trace_event/trace_event.h (right):

https://codereview.chromium.org/1151353003/diff/60001/base/trace_event/trace_...
base/trace_event/trace_event.h:764: id, timestamp)        \
On 2015/05/27 12:11:56, Sami wrote:
> Was this git cl format? I wonder if we should be following the existing style
> here instead.

Yeah this was git cl format. Went back to old formatting, but we should probably
reformat this whole file with git cl format at some point to avoid this in the
future.

https://codereview.chromium.org/1151353003/diff/60001/components/scheduler/ch...
File components/scheduler/child/idle_helper.cc (right):

https://codereview.chromium.org/1151353003/diff/60001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:127: if (helper_->IsShutdown())
On 2015/05/27 13:04:17, alexclarke1 wrote:
> Should we have a test for this?

Ack - I've not yet added any tests - will do that with the next patch-set.
(just to clarify, this wasn't a bug in the previous implementation - it only
became necessary with the new implementation).

https://codereview.chromium.org/1151353003/diff/60001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:308: DCHECK(new_deadline ==
idle_period_deadline_);
On 2015/05/27 12:11:56, Sami wrote:
> nit: DCHECK_EQ

crbug.com/463869 :(

https://codereview.chromium.org/1151353003/diff/60001/components/scheduler/ch...
File components/scheduler/child/idle_helper.h (right):

https://codereview.chromium.org/1151353003/diff/60001/components/scheduler/ch...
components/scheduler/child/idle_helper.h:156: // Do not modify these field
directly, instead call SetIdlePeriodState.
On 2015/05/27 13:04:17, alexclarke1 wrote:
> A random driveby thought.  If we wanted to enforce this (here and elsewhere),
> perhaps it's worth defining a struct to hold these with suitable accessors
> (SetIdlePeriodState would likely be a member of this struct)?

Good point - I've reworked it to encapsulate all this state (and a lot of the
tracing) into a separate internal State object.

[rmcilroy, 2015-05-27 19:26:31]

Patchset #4 (id:80001) has been deleted

[Sami, 2015-05-29 09:38:07]

Looks pretty good now I think.

https://codereview.chromium.org/1151353003/diff/60001/components/scheduler/ch...
File components/scheduler/child/idle_helper.cc (right):

https://codereview.chromium.org/1151353003/diff/60001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:308: DCHECK(new_deadline ==
idle_period_deadline_);
On 2015/05/27 19:17:05, rmcilroy wrote:
> On 2015/05/27 12:11:56, Sami wrote:
> > nit: DCHECK_EQ
> 
> crbug.com/463869 :(

I thought we could still use it for TimeTicks?

https://codereview.chromium.org/1151353003/diff/100001/components/scheduler/c...
File components/scheduler/child/idle_helper.cc (right):

https://codereview.chromium.org/1151353003/diff/100001/components/scheduler/c...
components/scheduler/child/idle_helper.cc:48: helper_->AddTaskObserver(this);
Please also remove this in the destructor.

https://codereview.chromium.org/1151353003/diff/100001/components/scheduler/c...
components/scheduler/child/idle_helper.cc:339: void
IdleHelper::State::WillProcessIdleTask() const {
Should we call these something like TraceIdleTaskStart/End() since they're not
really doing anything else as far as I can tell? Generally it feels like the
state should be dumb without any complex logic in it and naming the functions
accordingly could help to keep it that way.

https://codereview.chromium.org/1151353003/diff/100001/components/scheduler/c...
File components/scheduler/child/idle_helper.h (right):

https://codereview.chromium.org/1151353003/diff/100001/components/scheduler/c...
components/scheduler/child/idle_helper.h:162: mutable base::TimeTicks
last_traced_start_running_idle_task_;
I'm having trouble parsing this name. last_idle_task_trace_time or something?

[alex clarke (OOO till 29th), 2015-05-29 13:24:49]

https://codereview.chromium.org/1151353003/diff/100001/components/scheduler/c...
File components/scheduler/child/idle_helper.h (right):

https://codereview.chromium.org/1151353003/diff/100001/components/scheduler/c...
components/scheduler/child/idle_helper.h:142: helper_->CheckOnValidThread();
If you're going to define this inline, you should include the SchedulerHelper
header rather than forward declaring it.

[rmcilroy, 2015-06-01 15:33:21]

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

[rmcilroy, 2015-06-01 15:33:22]

+primiano

Primiano: could you take a look at the base/trace_event changes please.

Alex: could you check that the changes to the WorkerScheduler unittests make
sense.

PTAL, thanks.

https://codereview.chromium.org/1151353003/diff/60001/components/scheduler/ch...
File components/scheduler/child/idle_helper.cc (right):

https://codereview.chromium.org/1151353003/diff/60001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:127: if (helper_->IsShutdown())
On 2015/05/27 19:17:05, rmcilroy wrote:
> On 2015/05/27 13:04:17, alexclarke1 wrote:
> > Should we have a test for this?
> 
> Ack - I've not yet added any tests - will do that with the next patch-set.
> (just to clarify, this wasn't a bug in the previous implementation - it only
> became necessary with the new implementation).

Added TestLongIdlePeriodWhenShutdown in latest patch-set.

https://codereview.chromium.org/1151353003/diff/60001/components/scheduler/ch...
components/scheduler/child/idle_helper.cc:308: DCHECK(new_deadline ==
idle_period_deadline_);
On 2015/05/29 09:38:06, Sami wrote:
> On 2015/05/27 19:17:05, rmcilroy wrote:
> > On 2015/05/27 12:11:56, Sami wrote:
> > > nit: DCHECK_EQ
> > 
> > crbug.com/463869 :(
> 
> I thought we could still use it for TimeTicks?

Oops, was looking at the wrong line... Done.

https://codereview.chromium.org/1151353003/diff/100001/components/scheduler/c...
File components/scheduler/child/idle_helper.cc (right):

https://codereview.chromium.org/1151353003/diff/100001/components/scheduler/c...
components/scheduler/child/idle_helper.cc:48: helper_->AddTaskObserver(this);
On 2015/05/29 09:38:06, Sami wrote:
> Please also remove this in the destructor.

Done.

https://codereview.chromium.org/1151353003/diff/100001/components/scheduler/c...
components/scheduler/child/idle_helper.cc:339: void
IdleHelper::State::WillProcessIdleTask() const {
On 2015/05/29 09:38:06, Sami wrote:
> Should we call these something like TraceIdleTaskStart/End() since they're not
> really doing anything else as far as I can tell? Generally it feels like the
> state should be dumb without any complex logic in it and naming the functions
> accordingly could help to keep it that way.

SGTM, done.

https://codereview.chromium.org/1151353003/diff/100001/components/scheduler/c...
File components/scheduler/child/idle_helper.h (right):

https://codereview.chromium.org/1151353003/diff/100001/components/scheduler/c...
components/scheduler/child/idle_helper.h:142: helper_->CheckOnValidThread();
On 2015/05/29 13:24:49, alexclarke1 wrote:
> If you're going to define this inline, you should include the SchedulerHelper
> header rather than forward declaring it.

I moved it out-of-line.

https://codereview.chromium.org/1151353003/diff/100001/components/scheduler/c...
components/scheduler/child/idle_helper.h:162: mutable base::TimeTicks
last_traced_start_running_idle_task_;
On 2015/05/29 09:38:06, Sami wrote:
> I'm having trouble parsing this name. last_idle_task_trace_time or something?

Done.

[rmcilroy, 2015-06-01 15:33:38]

Patchset #5 (id:120001) has been deleted

[rmcilroy, 2015-06-01 15:35:26]

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

[commit-bot: I haz the power, 2015-06-01 15:36:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1151353003/140001

[commit-bot: I haz the power, 2015-06-01 15:36:16]

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

[commit-bot: I haz the power, 2015-06-01 15:36:17]

Dry run: CLs for remote refs other than refs/pending/heads/master must contain
NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them

[alex clarke (OOO till 29th), 2015-06-01 16:16:56]

LGTM for the test changes.

[Sami, 2015-06-01 17:11:58]

Thanks Ross. Couple of nits and an annoying question about threading.

https://codereview.chromium.org/1151353003/diff/140001/components/scheduler/c...
File components/scheduler/child/idle_helper.cc (right):

https://codereview.chromium.org/1151353003/diff/140001/components/scheduler/c...
components/scheduler/child/idle_helper.cc:251: if
(state_.idle_period_paused_flag()) {
Since we're reading this without a lock, could we end up in a case where we
think the long idle periods are running but they actually got paused right when
this task is posted and we end up not enabling the long idle period?

Alternatively this could check if we're on the main thread and post a helper to
check the state if not. WDYT?

https://codereview.chromium.org/1151353003/diff/140001/components/scheduler/c...
File components/scheduler/child/idle_helper.h (right):

https://codereview.chromium.org/1151353003/diff/140001/components/scheduler/c...
components/scheduler/child/idle_helper.h:144: bool idle_period_paused_flag()
const;
nit: Since this involves a little more than just reading a field, how about
calling it something like IdlePeriodPaused() ?

https://codereview.chromium.org/1151353003/diff/140001/components/scheduler/c...
components/scheduler/child/idle_helper.h:155: mutable base::TimeTicks
last_idle_task_trace_time_;
Rather than make these mutable, how about just making the necessary methods
non-const?

[Primiano Tucci (use gerrit), 2015-06-01 17:20:04]

base/trace_event LGTM

https://codereview.chromium.org/1151353003/diff/140001/base/trace_event/trace...
File base/trace_event/trace_event.h (right):

https://codereview.chromium.org/1151353003/diff/140001/base/trace_event/trace...
base/trace_event/trace_event.h:763: #define
TRACE_EVENT_NESTABLE_ASYNC_BEGIN_WITH_TIMESTAMP0(category_group, name, \
Ok there seems to be enough precedent for this.

Can you just add a comment saying:
// Similar to RACE_EVENT_NESTABLE_ASYNC_{BEGIN,END}X but with a custom
|timestamp| provided.

https://codereview.chromium.org/1151353003/diff/140001/components/scheduler/c...
File components/scheduler/child/idle_helper.cc (right):

https://codereview.chromium.org/1151353003/diff/140001/components/scheduler/c...
components/scheduler/child/idle_helper.cc:378: tracing_category_,
"DeadlineOverrun", this,
Just to doublecheck, the logic here would be that if you detect that an idle
task overrun, you'd show in tracing a rect that spans:

- from the deadline to the actual end of the task, if the task started before
the deadline.
or
- from the actual start to the actual end if the task started after the deadline
(I guess so it doesn't get falsely over-accounted if it wasn't given a
possibility to start).

[rmcilroy, 2015-06-01 19:12:43]

https://codereview.chromium.org/1151353003/diff/140001/base/trace_event/trace...
File base/trace_event/trace_event.h (right):

https://codereview.chromium.org/1151353003/diff/140001/base/trace_event/trace...
base/trace_event/trace_event.h:763: #define
TRACE_EVENT_NESTABLE_ASYNC_BEGIN_WITH_TIMESTAMP0(category_group, name, \
On 2015/06/01 17:20:04, Primiano Tucci wrote:
> Ok there seems to be enough precedent for this.
> 
> Can you just add a comment saying:
> // Similar to RACE_EVENT_NESTABLE_ASYNC_{BEGIN,END}X but with a custom
> |timestamp| provided.

Done.

https://codereview.chromium.org/1151353003/diff/140001/components/scheduler/c...
File components/scheduler/child/idle_helper.cc (right):

https://codereview.chromium.org/1151353003/diff/140001/components/scheduler/c...
components/scheduler/child/idle_helper.cc:251: if
(state_.idle_period_paused_flag()) {
On 2015/06/01 17:11:57, Sami wrote:
> Since we're reading this without a lock, could we end up in a case where we
> think the long idle periods are running but they actually got paused right
when
> this task is posted and we end up not enabling the long idle period?
> 
> Alternatively this could check if we're on the main thread and post a helper
to
> check the state if not. WDYT?

I was trying to avoid having to post a task here for all idle tasks posted from
another thread, but maybe you are right - when I think about it I would need to
extend the locking quite a bit further to be absolutely safe, so it's probably
better just to post a task when not on the main thread. Done.

https://codereview.chromium.org/1151353003/diff/140001/components/scheduler/c...
components/scheduler/child/idle_helper.cc:378: tracing_category_,
"DeadlineOverrun", this,
On 2015/06/01 17:20:04, Primiano Tucci wrote:
> Just to doublecheck, the logic here would be that if you detect that an idle
> task overrun, you'd show in tracing a rect that spans:
> 
> - from the deadline to the actual end of the task, if the task started before
> the deadline.
> or
> - from the actual start to the actual end if the task started after the
deadline
> (I guess so it doesn't get falsely over-accounted if it wasn't given a
> possibility to start).

Yes exactly. The second case is not only due to over-accounting, but also due to
the fact that the NESTABLE_ASYNC trace events get horribly confused if you try
to nest something inside an outer trace event which starts before that outer
trace event (which would happen if I didn't limit the block to the actual start
time here).

https://codereview.chromium.org/1151353003/diff/140001/components/scheduler/c...
File components/scheduler/child/idle_helper.h (right):

https://codereview.chromium.org/1151353003/diff/140001/components/scheduler/c...
components/scheduler/child/idle_helper.h:144: bool idle_period_paused_flag()
const;
On 2015/06/01 17:11:57, Sami wrote:
> nit: Since this involves a little more than just reading a field, how about
> calling it something like IdlePeriodPaused() ?

Ack (doesn't exist any longer).

https://codereview.chromium.org/1151353003/diff/140001/components/scheduler/c...
components/scheduler/child/idle_helper.h:155: mutable base::TimeTicks
last_idle_task_trace_time_;
On 2015/06/01 17:11:57, Sami wrote:
> Rather than make these mutable, how about just making the necessary methods
> non-const?

Ok. This makes WillProcessIdleTask non-const though.

[rmcilroy, 2015-06-01 19:13:44]

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

[rmcilroy, 2015-06-01 19:13:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, alexclarke@chromium.org
Link to the patchset: https://codereview.chromium.org/1151353003/#ps180001
(title: "Address review comments.")

[commit-bot: I haz the power, 2015-06-01 19:16:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1151353003/180001

[commit-bot: I haz the power, 2015-06-01 21:10:27]

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

[commit-bot: I haz the power, 2015-06-01 21:10:29]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Sami, 2015-06-02 10:33:16]

Thanks Ross, lgtm!

https://codereview.chromium.org/1151353003/diff/140001/components/scheduler/c...
File components/scheduler/child/idle_helper.cc (right):

https://codereview.chromium.org/1151353003/diff/140001/components/scheduler/c...
components/scheduler/child/idle_helper.cc:251: if
(state_.idle_period_paused_flag()) {
On 2015/06/01 19:12:43, rmcilroy wrote:
> On 2015/06/01 17:11:57, Sami wrote:
> > Since we're reading this without a lock, could we end up in a case where we
> > think the long idle periods are running but they actually got paused right
> when
> > this task is posted and we end up not enabling the long idle period?
> > 
> > Alternatively this could check if we're on the main thread and post a helper
> to
> > check the state if not. WDYT?
> 
> I was trying to avoid having to post a task here for all idle tasks posted
from
> another thread, but maybe you are right - when I think about it I would need
to
> extend the locking quite a bit further to be absolutely safe, so it's probably
> better just to post a task when not on the main thread. Done.

Thanks, this looks better. I guess the use case of idle work coming from another
thread isn't that common anyway because generally idle tasks will need to repost
themselves on the main thread.

https://codereview.chromium.org/1151353003/diff/140001/components/scheduler/c...
File components/scheduler/child/idle_helper.h (right):

https://codereview.chromium.org/1151353003/diff/140001/components/scheduler/c...
components/scheduler/child/idle_helper.h:155: mutable base::TimeTicks
last_idle_task_trace_time_;
On 2015/06/01 19:12:43, rmcilroy wrote:
> On 2015/06/01 17:11:57, Sami wrote:
> > Rather than make these mutable, how about just making the necessary methods
> > non-const?
> 
> Ok. This makes WillProcessIdleTask non-const though.

Right, that seems fine since it's meant to be an arbitrary callback anyway.

https://codereview.chromium.org/1151353003/diff/180001/components/scheduler/c...
File components/scheduler/child/idle_helper.cc (right):

https://codereview.chromium.org/1151353003/diff/180001/components/scheduler/c...
components/scheduler/child/idle_helper.cc:184:
on_idle_task_posted_closure_.Cancel();
I *think* this will never falsely prevent an idle task from running because
someone will need to start an idle period after this function for any idle work
to happen, and doing that will effectively do what the canceled closure would've
done. Right?-)

[rmcilroy, 2015-06-02 13:35:08]

Patchset #8 (id:200001) has been deleted

[rmcilroy, 2015-06-02 13:35:12]

https://codereview.chromium.org/1151353003/diff/180001/components/scheduler/c...
File components/scheduler/child/idle_helper.cc (right):

https://codereview.chromium.org/1151353003/diff/180001/components/scheduler/c...
components/scheduler/child/idle_helper.cc:184:
on_idle_task_posted_closure_.Cancel();
On 2015/06/02 10:33:15, Sami wrote:
> I *think* this will never falsely prevent an idle task from running because
> someone will need to start an idle period after this function for any idle
work
> to happen, and doing that will effectively do what the canceled closure
would've
> done. Right?-)

Right - If we are calling end idle period then we want to ensure that we stay
out of idle period mode until something explicitly calls start idle period
again. When they call that then the queue will be pumped anyway, so we won't
miss any idle tasks.

[rmcilroy, 2015-06-02 13:35:20]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-06-02 13:35:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, skyostil@chromium.org, alexclarke@chromium.org
Link to the patchset: https://codereview.chromium.org/1151353003/#ps220001
(title: "Fix ASAN and Windows")

[commit-bot: I haz the power, 2015-06-02 13:36:00]

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

[rmcilroy, 2015-06-02 15:00:32]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-06-02 15:00:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, skyostil@chromium.org, alexclarke@chromium.org
Link to the patchset: https://codereview.chromium.org/1151353003/#ps240001
(title: "Fix Win for realz hopefully...")

[commit-bot: I haz the power, 2015-06-02 15:01:04]

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

[commit-bot: I haz the power, 2015-06-02 17:13:05]

Committed patchset #9 (id:240001)

[commit-bot: I haz the power, 2015-06-02 17:13:55]

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