Refactor TimeBase to post tasks. Workers to use real Idle tasks.

This patch refactors TimerBase to post tasks, and deletes the
obsolete Blink TimerHeap.  Unfortunately this meant refactoring
WorkerThreads too (ideally that would have been done in a
separate patch).  I have change them to use the new
WorkerScheduler's idle tasks for v8 GC.

ATTN Sheriffs: If there are weird layout test flakes all of a
sudden, this patch may be the cause since the interleaving of 
blink timers with other chromium tasks will change.


BUG=463143, 416362, 480522

[Sami, 2015-02-27 18:29:52]

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

[Sami, 2015-02-27 18:29:52]

https://codereview.chromium.org/956333002/diff/60001/Source/core/dom/ScriptRu...
File Source/core/dom/ScriptRunnerTest.cpp (right):

https://codereview.chromium.org/956333002/diff/60001/Source/core/dom/ScriptRu...
Source/core/dom/ScriptRunnerTest.cpp:90: Scheduler::shutdown();
I wonder if we should have some testing helper for temporarily replacing the
Platform? It's spectacularly easy to get this wrong.

[alex clarke (OOO till 29th), 2015-03-19 11:34:50]

Patchset #7 (id:120001) has been deleted

[Sami, 2015-04-02 10:59:19]

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

[Sami, 2015-04-02 10:59:20]

If there's any way to split out killing the timer heap from implementing idle
work on the worker thread, let's do that since there are a lot of moving parts
here.

https://codereview.chromium.org/956333002/diff/160001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/956333002/diff/160001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:237: class WorkerThreadWaitUntilIdleTask :
public WebThread::Task, public WebThread::IdleTask  {
Maybe "wait until quiescence" would be a better term since idle is very
overloaded here.

https://codereview.chromium.org/956333002/diff/160001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:338: dontStartIdleTaskYet();
endIdlePeriod() to match the chromium side terms?

https://codereview.chromium.org/956333002/diff/160001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/956333002/diff/160001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.h:135: void dontStartIdleTaskYet();
Should these be talking about the idle period instead of a task?

[alex clarke (OOO till 29th), 2015-04-08 15:43:35]

I'm not entirely sure how to land the Worker idle changes separately (they'd
have to go first since TimerBase refactor unavoidably affects the main thread
and workers).

I guess it might be possible to get the WorkerSharedTimer to somehow post tasks
onto the new WorkerScheduler.  Looking at the chromium implementation of
SharedTimer there seems to be one timer shared across all threads (that seems
somewhat odd), unless it turns out there's one BlinkPlatformImpl per thread.

If it turns out that there is a lot of (temporary) plumbing on the chromium side
to support splitting this up I'd argue against doing so.

https://codereview.chromium.org/956333002/diff/160001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/956333002/diff/160001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:237: class WorkerThreadWaitUntilIdleTask :
public WebThread::Task, public WebThread::IdleTask  {
On 2015/04/02 10:59:19, Sami wrote:
> Maybe "wait until quiescence" would be a better term since idle is very
> overloaded here.

Done.

https://codereview.chromium.org/956333002/diff/160001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:338: dontStartIdleTaskYet();
On 2015/04/02 10:59:19, Sami wrote:
> endIdlePeriod() to match the chromium side terms?

I'm not really convinced those terms are helpful in blink.  They have well
defined meanings in chromium, but this business of not starting idle tasks is
pretty much a hack.  I'd like to see it go away since IMO only the scheduler
should know anything about idle periods.

Besides, surely dontStartIdleTaskYet accurately describes what it's doing?

[Sami, 2015-04-09 10:52:30]

Fair enough if there's no easy way to split this up. I've added one suggestion
how to reduce the size anyway.

https://codereview.chromium.org/956333002/diff/200001/Source/core/dom/ScriptR...
File Source/core/dom/ScriptRunnerTest.cpp (right):

https://codereview.chromium.org/956333002/diff/200001/Source/core/dom/ScriptR...
Source/core/dom/ScriptRunnerTest.cpp:89: Scheduler::shutdown();
Why is this change needed?

https://codereview.chromium.org/956333002/diff/200001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/956333002/diff/200001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:236: // TODO(scheduler-dev): Ideally the
WorkerScheduler should determine Quiescence rather than doing it here.
nit: I think we should use real usernames for TODOs since I don't see anyone
else using aliases like this.

https://codereview.chromium.org/956333002/diff/200001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:340: maybeStartIdleTask();
This reads a little poorly ("don't do idle ... oh wait I might have changed my
mind so maybe do it after all"). Should we add something like
startPollingForQuietPeriod?

https://codereview.chromium.org/956333002/diff/200001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:508: // check every
|m_minQuietPeriodMsBeforeDoingIdleGc| if any tasks have been run.  If they
havent we can start doing idle GCs.
s/havent/haven't/

https://codereview.chromium.org/956333002/diff/200001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:509: // NOTE we use the IdleTaskAfterWakeup
call to work out if something has run.  It's execution
nit: Its

https://codereview.chromium.org/956333002/diff/200001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/956333002/diff/200001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.h:132: void
setMinQuietPeriodBeforeDoingIdleGcForTesting(long long
minQuietPeriodBeforeDoingIdleGc)
For better or worse, "double seconds" is the preferred monotonic time data type
in Blink. I'm not sure why delays use long long but IMO that shouldn't be needed
here (feel free to keep in long long internally if you want). Also please make
the unit obvious in the function and parameter names.

https://codereview.chromium.org/956333002/diff/200001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.h:148: void dontStartIdleTaskYet();
Okay, I think I agree that it's probably better not to talk about idle periods
here. However the naming could be a little more consistent since you don't
"start" tasks but either "post" or "run" them. How about
dontPostIdleTaskYet()/maybePostIdleTask(), or better yet:
dontStartIdleGcYet()/maybeStartIdleGc()/idleGc()?

https://codereview.chromium.org/956333002/diff/200001/Source/core/workers/Wor...
File Source/core/workers/WorkerThreadTest.cpp (right):

https://codereview.chromium.org/956333002/diff/200001/Source/core/workers/Wor...
Source/core/workers/WorkerThreadTest.cpp:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
If we want to make this patch smaller, landing this test scaffolding first is
one option.

https://codereview.chromium.org/956333002/diff/200001/Source/core/workers/Wor...
Source/core/workers/WorkerThreadTest.cpp:177:
EXPECT_CALL(*m_mockWorkerReportingProxy, workerGlobalScopeStarted(_)).Times(1);
Could these be moved to a helper setup function? They don't seem related to idle
gc.

https://codereview.chromium.org/956333002/diff/200001/Source/platform/ThreadT...
File Source/platform/ThreadTimers.cpp (left):

https://codereview.chromium.org/956333002/diff/200001/Source/platform/ThreadT...
Source/platform/ThreadTimers.cpp:103: TRACE_EVENT_SET_SAMPLING_STATE("blink",
"BlinkInternal");
I assume this is for sampled trace events? Can we keep it working?

https://codereview.chromium.org/956333002/diff/200001/Source/platform/Timer.cpp
File Source/platform/Timer.cpp (right):

https://codereview.chromium.org/956333002/diff/200001/Source/platform/Timer.c...
Source/platform/Timer.cpp:94: // Keep heap valid while changing the next-fire
time.
This comment looks out of date.

https://codereview.chromium.org/956333002/diff/200001/Source/platform/Timer.c...
Source/platform/Timer.cpp:107: // Currently only the main thread has a
scheduler, so we need to check which thread we are posting from.
Do we still need this distinction?

https://codereview.chromium.org/956333002/diff/200001/Source/platform/Timer.c...
Source/platform/Timer.cpp:137: ASSERT(false && "IMPLEMENT ME!");
Any thoughts on how to implement this? Is this used by the inspector or
something?

https://codereview.chromium.org/956333002/diff/200001/Source/platform/Timer.h
File Source/platform/Timer.h (right):

https://codereview.chromium.org/956333002/diff/200001/Source/platform/Timer.h...
Source/platform/Timer.h:83: // NOTE we can't use CancellableTaskFactory here,
because the destructor of
"Some memory access" sounds a little vague. IIRC one of the problems was that
CancellableTaskFactory will have a pointer to the PartitionAlloc heap, which
isn't there anymore after Blink shuts down. Is that the problem with the unit
tests too or was that something else?

https://codereview.chromium.org/956333002/diff/200001/Source/platform/schedul...
File Source/platform/scheduler/Scheduler.cpp (right):

https://codereview.chromium.org/956333002/diff/200001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.cpp:92: ASSERT(WTF::isMainThread());
The comment says this function can be called from any thread?

https://codereview.chromium.org/956333002/diff/200001/Source/platform/schedul...
File Source/platform/scheduler/Scheduler.h (right):

https://codereview.chromium.org/956333002/diff/200001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.h:51: // For Blink Timer tasks, e.g.
DOMTimers,  Can only be called from the blink main thread.
nit: s/C/c/ or start a new sentence.

[alex clarke (OOO till 29th), 2015-04-10 15:29:42]

Many of the comment no longer apply after moving the quiescence check to
chromium.

https://codereview.chromium.org/956333002/diff/200001/Source/core/dom/ScriptR...
File Source/core/dom/ScriptRunnerTest.cpp (right):

https://codereview.chromium.org/956333002/diff/200001/Source/core/dom/ScriptR...
Source/core/dom/ScriptRunnerTest.cpp:89: Scheduler::shutdown();
On 2015/04/09 10:52:29, Sami wrote:
> Why is this change needed

I think this was due to some shutdown problems.  Fortunately this no longer
seems to be needed.

https://codereview.chromium.org/956333002/diff/200001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/956333002/diff/200001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:236: // TODO(scheduler-dev): Ideally the
WorkerScheduler should determine Quiescence rather than doing it here.
On 2015/04/09 10:52:29, Sami wrote:
> nit: I think we should use real usernames for TODOs since I don't see anyone
> else using aliases like this.

Acknowledged.

https://codereview.chromium.org/956333002/diff/200001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:340: maybeStartIdleTask();
On 2015/04/09 10:52:29, Sami wrote:
> This reads a little poorly ("don't do idle ... oh wait I might have changed my
> mind so maybe do it after all"). Should we add something like
> startPollingForQuietPeriod?

Acknowledged.

https://codereview.chromium.org/956333002/diff/200001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:508: // check every
|m_minQuietPeriodMsBeforeDoingIdleGc| if any tasks have been run.  If they
havent we can start doing idle GCs.
On 2015/04/09 10:52:29, Sami wrote:
> s/havent/haven't/

Acknowledged.

https://codereview.chromium.org/956333002/diff/200001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:509: // NOTE we use the IdleTaskAfterWakeup
call to work out if something has run.  It's execution
On 2015/04/09 10:52:29, Sami wrote:
> nit: Its

Acknowledged.

https://codereview.chromium.org/956333002/diff/200001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/956333002/diff/200001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.h:132: void
setMinQuietPeriodBeforeDoingIdleGcForTesting(long long
minQuietPeriodBeforeDoingIdleGc)
On 2015/04/09 10:52:29, Sami wrote:
> For better or worse, "double seconds" is the preferred monotonic time data
type
> in Blink. I'm not sure why delays use long long but IMO that shouldn't be
needed
> here (feel free to keep in long long internally if you want). Also please make
> the unit obvious in the function and parameter names.

Acknowledged.

https://codereview.chromium.org/956333002/diff/200001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.h:148: void dontStartIdleTaskYet();
On 2015/04/09 10:52:29, Sami wrote:
> Okay, I think I agree that it's probably better not to talk about idle periods
> here. However the naming could be a little more consistent since you don't
> "start" tasks but either "post" or "run" them. How about
> dontPostIdleTaskYet()/maybePostIdleTask(), or better yet:
> dontStartIdleGcYet()/maybeStartIdleGc()/idleGc()?

Acknowledged.

https://codereview.chromium.org/956333002/diff/200001/Source/core/workers/Wor...
File Source/core/workers/WorkerThreadTest.cpp (right):

https://codereview.chromium.org/956333002/diff/200001/Source/core/workers/Wor...
Source/core/workers/WorkerThreadTest.cpp:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/04/09 10:52:29, Sami wrote:
> If we want to make this patch smaller, landing this test scaffolding first is
> one option.

Acknowledged.

https://codereview.chromium.org/956333002/diff/200001/Source/core/workers/Wor...
Source/core/workers/WorkerThreadTest.cpp:177:
EXPECT_CALL(*m_mockWorkerReportingProxy, workerGlobalScopeStarted(_)).Times(1);
On 2015/04/09 10:52:29, Sami wrote:
> Could these be moved to a helper setup function? They don't seem related to
idle
> gc.

Done.

https://codereview.chromium.org/956333002/diff/200001/Source/platform/ThreadT...
File Source/platform/ThreadTimers.cpp (left):

https://codereview.chromium.org/956333002/diff/200001/Source/platform/ThreadT...
Source/platform/ThreadTimers.cpp:103: TRACE_EVENT_SET_SAMPLING_STATE("blink",
"BlinkInternal");
On 2015/04/09 10:52:29, Sami wrote:
> I assume this is for sampled trace events? Can we keep it working?

Done.

https://codereview.chromium.org/956333002/diff/200001/Source/platform/Timer.cpp
File Source/platform/Timer.cpp (right):

https://codereview.chromium.org/956333002/diff/200001/Source/platform/Timer.c...
Source/platform/Timer.cpp:137: ASSERT(false && "IMPLEMENT ME!");
On 2015/04/09 10:52:29, Sami wrote:
> Any thoughts on how to implement this? Is this used by the inspector or
> something?

The callsite for this has gone away, yay!

https://codereview.chromium.org/956333002/diff/200001/Source/platform/Timer.h
File Source/platform/Timer.h (right):

https://codereview.chromium.org/956333002/diff/200001/Source/platform/Timer.h...
Source/platform/Timer.h:83: // NOTE we can't use CancellableTaskFactory here,
because the destructor of
On 2015/04/09 10:52:29, Sami wrote:
> "Some memory access" sounds a little vague. IIRC one of the problems was that
> CancellableTaskFactory will have a pointer to the PartitionAlloc heap, which
> isn't there anymore after Blink shuts down. Is that the problem with the unit
> tests too or was that something else?

It was with the unit tests.  Clarified the comment slightly.

https://codereview.chromium.org/956333002/diff/200001/Source/platform/schedul...
File Source/platform/scheduler/Scheduler.cpp (right):

https://codereview.chromium.org/956333002/diff/200001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.cpp:92: ASSERT(WTF::isMainThread());
On 2015/04/09 10:52:30, Sami wrote:
> The comment says this function can be called from any thread?

Good point, removing.

https://codereview.chromium.org/956333002/diff/200001/Source/platform/schedul...
File Source/platform/scheduler/Scheduler.h (right):

https://codereview.chromium.org/956333002/diff/200001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.h:51: // For Blink Timer tasks, e.g.
DOMTimers,  Can only be called from the blink main thread.
On 2015/04/09 10:52:30, Sami wrote:
> nit: s/C/c/ or start a new sentence.

Done.

[rmcilroy, 2015-04-13 13:55:07]

https://codereview.chromium.org/956333002/diff/220001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (left):

https://codereview.chromium.org/956333002/diff/220001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:515: int64_t delay =
kLongIdleHandlerDelayMs;
I've just realized that this was previously sending a 1s idle notification to
the GC. The V8 GC does some GC steps (e.g. compaction) only when the idle
notification is ~1s, so with the new approach we aren't going to get any
compaction on the worker which is bad.
I have an OKR for this Q2 which would allow us to do "abortable" compaction
which would avoid this need, however for now I think we are going to need to
keep some kind of long 1s GC tasks to enable webworkers to shrink their memory.

Maybe as a temporary measure we can use the 
CanExceedIdleDeadlineIfRequired in the idle task and send a 1s idle time if this
returns true.

[alex clarke (OOO till 29th), 2015-04-14 13:04:35]

https://codereview.chromium.org/956333002/diff/220001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (left):

https://codereview.chromium.org/956333002/diff/220001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:515: int64_t delay =
kLongIdleHandlerDelayMs;
On 2015/04/13 13:55:07, rmcilroy wrote:
> I've just realized that this was previously sending a 1s idle notification to
> the GC. The V8 GC does some GC steps (e.g. compaction) only when the idle
> notification is ~1s, so with the new approach we aren't going to get any
> compaction on the worker which is bad.
> I have an OKR for this Q2 which would allow us to do "abortable" compaction
> which would avoid this need, however for now I think we are going to need to
> keep some kind of long 1s GC tasks to enable webworkers to shrink their
memory.
> 
> Maybe as a temporary measure we can use the 
> CanExceedIdleDeadlineIfRequired in the idle task and send a 1s idle time if
this
> returns true.

OK I can do that, but I need to wire CanExceedIdleDeadlineIfRequired into
WebThread and add the plumbing in chromium.

[alex clarke (OOO till 29th), 2015-04-15 15:15:39]

PTAL

[Sami, 2015-04-16 14:23:39]

Some minor comments.

https://codereview.chromium.org/956333002/diff/260001/Source/core/workers/Wor...
File Source/core/workers/WorkerLoaderProxy.h (right):

https://codereview.chromium.org/956333002/diff/260001/Source/core/workers/Wor...
Source/core/workers/WorkerLoaderProxy.h:88: protected:
Could you add a comment saying this is for testing?

https://codereview.chromium.org/956333002/diff/260001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/956333002/diff/260001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:301:
m_webScheduler->postIdleTaskAfterWakeup(FROM_HERE, new
WorkerThreadIdleTask(this));
There's no way this can run after this thread object has been destroyed, right?

https://codereview.chromium.org/956333002/diff/260001/Source/core/workers/Wor...
File Source/core/workers/WorkerThreadTest.cpp (right):

https://codereview.chromium.org/956333002/diff/260001/Source/core/workers/Wor...
Source/core/workers/WorkerThreadTest.cpp:189: bool gcDone = false;
Should this be some kind of thread safe primitive instead, or at least volatile?
Ditto for the other tests.

https://codereview.chromium.org/956333002/diff/260001/Source/core/workers/Wor...
Source/core/workers/WorkerThreadTest.cpp:243: while (taskCount < 10)
Is this thread safe?

https://codereview.chromium.org/956333002/diff/260001/Source/platform/Testing...
File Source/platform/TestingPlatformSupport.cpp (right):

https://codereview.chromium.org/956333002/diff/260001/Source/platform/Testing...
Source/platform/TestingPlatformSupport.cpp:70: fprintf(stderr,
"TestingPlatformSupport\n");
Left in by mistake?

https://codereview.chromium.org/956333002/diff/260001/Source/platform/Timer.cpp
File Source/platform/Timer.cpp (right):

https://codereview.chromium.org/956333002/diff/260001/Source/platform/Timer.c...
Source/platform/Timer.cpp:126: // NOTE repeating timers drift, but it's
preserving the functionality of the old TimerHeap.
Should we link to crbug.com/328700 here or is this a different issue?

https://codereview.chromium.org/956333002/diff/260001/Source/platform/Timer.c...
Source/platform/Timer.cpp:129: fired();
Is it worth having a trace event around this or is it obvious from a trace that
we're running a timer callback?

[alex clarke (OOO till 29th), 2015-04-16 17:20:01]

PTAL

https://codereview.chromium.org/956333002/diff/260001/Source/core/workers/Wor...
File Source/core/workers/WorkerLoaderProxy.h (right):

https://codereview.chromium.org/956333002/diff/260001/Source/core/workers/Wor...
Source/core/workers/WorkerLoaderProxy.h:88: protected:
On 2015/04/16 14:23:38, Sami wrote:
> Could you add a comment saying this is for testing?

Done.

https://codereview.chromium.org/956333002/diff/260001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/956333002/diff/260001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:301:
m_webScheduler->postIdleTaskAfterWakeup(FROM_HERE, new
WorkerThreadIdleTask(this));
On 2015/04/16 14:23:38, Sami wrote:
> There's no way this can run after this thread object has been destroyed,
right?

I don't think so because when the thread goes away the SchedulerHelper gets
deleted which deletes the TQM and the queues get flushed.

https://codereview.chromium.org/956333002/diff/260001/Source/core/workers/Wor...
File Source/core/workers/WorkerThreadTest.cpp (right):

https://codereview.chromium.org/956333002/diff/260001/Source/core/workers/Wor...
Source/core/workers/WorkerThreadTest.cpp:189: bool gcDone = false;
On 2015/04/16 14:23:38, Sami wrote:
> Should this be some kind of thread safe primitive instead, or at least
volatile?
> Ditto for the other tests.

I added some mutexes and a ThreadCondition

https://codereview.chromium.org/956333002/diff/260001/Source/core/workers/Wor...
Source/core/workers/WorkerThreadTest.cpp:243: while (taskCount < 10)
On 2015/04/16 14:23:38, Sami wrote:
> Is this thread safe?

Done.

https://codereview.chromium.org/956333002/diff/260001/Source/platform/Testing...
File Source/platform/TestingPlatformSupport.cpp (right):

https://codereview.chromium.org/956333002/diff/260001/Source/platform/Testing...
Source/platform/TestingPlatformSupport.cpp:70: fprintf(stderr,
"TestingPlatformSupport\n");
On 2015/04/16 14:23:38, Sami wrote:
> Left in by mistake?

Done.

https://codereview.chromium.org/956333002/diff/260001/Source/platform/Timer.cpp
File Source/platform/Timer.cpp (right):

https://codereview.chromium.org/956333002/diff/260001/Source/platform/Timer.c...
Source/platform/Timer.cpp:126: // NOTE repeating timers drift, but it's
preserving the functionality of the old TimerHeap.
On 2015/04/16 14:23:39, Sami wrote:
> Should we link to crbug.com/328700 here or is this a different issue?

Yeah that may be related.

https://codereview.chromium.org/956333002/diff/260001/Source/platform/Timer.c...
Source/platform/Timer.cpp:129: fired();
On 2015/04/16 14:23:39, Sami wrote:
> Is it worth having a trace event around this or is it obvious from a trace
that
> we're running a timer callback?

You can tell with out a trace event, but it would be easier if we add one here.

[rmcilroy, 2015-04-17 12:59:33]

Loving all the red :). A couple of comments but looks good overall for the bits
I understand.

https://codereview.chromium.org/956333002/diff/320001/LayoutTests/fast/events...
File
LayoutTests/fast/events/resources/subframe-stop-load-in-unload-handler-using-document-write.html
(right):

https://codereview.chromium.org/956333002/diff/320001/LayoutTests/fast/events...
LayoutTests/fast/events/resources/subframe-stop-load-in-unload-handler-using-document-write.html:7:
window.onload = function() {
Could we make the layouttest changes in a seperate CL (I assume they work both
with and without these changes?).

https://codereview.chromium.org/956333002/diff/320001/Source/core/dom/ScriptL...
File Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/956333002/diff/320001/Source/core/dom/ScriptL...
Source/core/dom/ScriptLoader.cpp:325: fprintf(stderr,
"ScriptLoader::executeScript\n");
leftover debugging?

https://codereview.chromium.org/956333002/diff/320001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/956333002/diff/320001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:301:
m_webScheduler->postIdleTaskAfterWakeup(FROM_HERE, new
WorkerThreadIdleTask(this));
Can we just use WTF::Bind here to avoid having to create the
WorkerThreadIdleTask class?

https://codereview.chromium.org/956333002/diff/320001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:465: gcDeadlineSeconds =
Platform::current()->monotonicallyIncreasingTime() + 1.0;
add back the kLongIdleHandlerDelay as a constant and use it here instead of the
literal '1.0'

https://codereview.chromium.org/956333002/diff/320001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:467: if (doIdleGc(gcDeadlineSeconds))
nit - I would prefer this function is inlined - i.e.: 
  bool gcFinished = false;
  if (deadlineSeconds > Platform::current()->monotonicallyIncreasingTime())
    gcFinished = isolate()->IdleNotificationDeadline(deadlineSeconds);

  if (gcFinished) {
    ...

I think this is a bit cleaner (and is similar to how V8Initilizer does it. WDYT?

https://codereview.chromium.org/956333002/diff/320001/Source/platform/Timer.h
File Source/platform/Timer.h (right):

https://codereview.chromium.org/956333002/diff/320001/Source/platform/Timer.h...
Source/platform/Timer.h:82: // CancellableTaskFactory::CancellableTask does some
memory access (causes various tests to crash on shtdown).
/s/shtdown/shutdown

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

https://codereview.chromium.org/956333002/diff/320001/LayoutTests/fast/events...
File
LayoutTests/fast/events/resources/subframe-stop-load-in-unload-handler-using-document-write.html
(right):

https://codereview.chromium.org/956333002/diff/320001/LayoutTests/fast/events...
LayoutTests/fast/events/resources/subframe-stop-load-in-unload-handler-using-document-write.html:7:
window.onload = function() {
On 2015/04/17 12:59:32, rmcilroy wrote:
> Could we make the layouttest changes in a seperate CL (I assume they work both
> with and without these changes?).

Possibly.  I think that might depend on who does the owners review.

https://codereview.chromium.org/956333002/diff/320001/Source/core/dom/ScriptL...
File Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/956333002/diff/320001/Source/core/dom/ScriptL...
Source/core/dom/ScriptLoader.cpp:325: fprintf(stderr,
"ScriptLoader::executeScript\n");
On 2015/04/17 12:59:32, rmcilroy wrote:
> leftover debugging?

Done.

https://codereview.chromium.org/956333002/diff/320001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/956333002/diff/320001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:301:
m_webScheduler->postIdleTaskAfterWakeup(FROM_HERE, new
WorkerThreadIdleTask(this));
On 2015/04/17 12:59:33, rmcilroy wrote:
> Can we just use WTF::Bind here to avoid having to create the
> WorkerThreadIdleTask class?

I wish I could but the type ereasure of WTF::bind for member function pointers
with parameters seems broken. I get errors like this if I try:

m_webScheduler->postIdleTaskAfterWakeup(FROM_HERE,
WTF::bind(&WorkerThread::idleTask, this));


../../third_party/WebKit/public/platform/WebScheduler.h:74:10: note: candidate
function not viable: no known conversion from 'PassOwnPtr<Function<typename
FunctionWrapper<void (WorkerThread::*)(double)>::ResultType ()>>' to
'PassOwnPtr<Function<void (double)>>' for 2nd argument
    void postIdleTaskAfterWakeup(const WebTraceLocation&, PassOwnPtr<IdleTask>);

https://codereview.chromium.org/956333002/diff/320001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:465: gcDeadlineSeconds =
Platform::current()->monotonicallyIncreasingTime() + 1.0;
On 2015/04/17 12:59:33, rmcilroy wrote:
> add back the kLongIdleHandlerDelay as a constant and use it here instead of
the
> literal '1.0'

Done.

https://codereview.chromium.org/956333002/diff/320001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:467: if (doIdleGc(gcDeadlineSeconds))
On 2015/04/17 12:59:33, rmcilroy wrote:
> nit - I would prefer this function is inlined - i.e.: 
>   bool gcFinished = false;
>   if (deadlineSeconds > Platform::current()->monotonicallyIncreasingTime())
>     gcFinished = isolate()->IdleNotificationDeadline(deadlineSeconds);
> 
>   if (gcFinished) {
>     ...
> 
> I think this is a bit cleaner (and is similar to how V8Initilizer does it.
WDYT?

I split the doIdleGc function off so I could test this.  Any idea how to test it
if I do what you suggest?

https://codereview.chromium.org/956333002/diff/320001/Source/platform/Timer.h
File Source/platform/Timer.h (right):

https://codereview.chromium.org/956333002/diff/320001/Source/platform/Timer.h...
Source/platform/Timer.h:82: // CancellableTaskFactory::CancellableTask does some
memory access (causes various tests to crash on shtdown).
On 2015/04/17 12:59:33, rmcilroy wrote:
> /s/shtdown/shutdown

Done.

[Sami, 2015-04-17 15:34:32]

Thanks Alex, lgtm. I'll let Ross okay the idle gc logic.

https://codereview.chromium.org/956333002/diff/340001/LayoutTests/fast/events...
File
LayoutTests/fast/events/resources/subframe-stop-load-in-unload-handler-using-document-write.html
(right):

https://codereview.chromium.org/956333002/diff/340001/LayoutTests/fast/events...
LayoutTests/fast/events/resources/subframe-stop-load-in-unload-handler-using-document-write.html:2:
// We need to wait until the iframe has finished loading before navigating or
These layout test tweaks could be landed separately to cut down on the moving
parts here.

https://codereview.chromium.org/956333002/diff/340001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/956333002/diff/340001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:253: RawPtr<WorkerThread> m_thread;
// Not owned?

[alex clarke (OOO till 29th), 2015-04-17 16:01:33]

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

[commit-bot: I haz the power, 2015-04-17 16:02:32]

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

[rmcilroy, 2015-04-17 16:33:22]

WorkerThread and WorkerThreadTest lgtm. Thanks!

https://codereview.chromium.org/956333002/diff/320001/LayoutTests/fast/events...
File
LayoutTests/fast/events/resources/subframe-stop-load-in-unload-handler-using-document-write.html
(right):

https://codereview.chromium.org/956333002/diff/320001/LayoutTests/fast/events...
LayoutTests/fast/events/resources/subframe-stop-load-in-unload-handler-using-document-write.html:7:
window.onload = function() {
On 2015/04/17 13:37:08, alexclarke1 wrote:
> On 2015/04/17 12:59:32, rmcilroy wrote:
> > Could we make the layouttest changes in a seperate CL (I assume they work
both
> > with and without these changes?).
> 
> Possibly.  I think that might depend on who does the owners review.

I'm not sure what you mean here. LayoutTest changes don't need OWNER's reviews
so you could land these changes independently of this change if it's possible (I
think this would make it clearer that the change shouldn't change behaviour well
written layouttests if it didn't modify any itself, but I don't really mind
either way).

https://codereview.chromium.org/956333002/diff/320001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/956333002/diff/320001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:301:
m_webScheduler->postIdleTaskAfterWakeup(FROM_HERE, new
WorkerThreadIdleTask(this));
On 2015/04/17 13:37:08, alexclarke1 wrote:
> On 2015/04/17 12:59:33, rmcilroy wrote:
> > Can we just use WTF::Bind here to avoid having to create the
> > WorkerThreadIdleTask class?
> 
> I wish I could but the type ereasure of WTF::bind for member function pointers
> with parameters seems broken. I get errors like this if I try:
> 
> m_webScheduler->postIdleTaskAfterWakeup(FROM_HERE,
> WTF::bind(&WorkerThread::idleTask, this));
> 
> 
> ../../third_party/WebKit/public/platform/WebScheduler.h:74:10: note: candidate
> function not viable: no known conversion from 'PassOwnPtr<Function<typename
> FunctionWrapper<void (WorkerThread::*)(double)>::ResultType ()>>' to
> 'PassOwnPtr<Function<void (double)>>' for 2nd argument
>     void postIdleTaskAfterWakeup(const WebTraceLocation&,
PassOwnPtr<IdleTask>);

Ahh sad. In that case this is fine.

https://codereview.chromium.org/956333002/diff/320001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:467: if (doIdleGc(gcDeadlineSeconds))
On 2015/04/17 13:37:08, alexclarke1 wrote:
> On 2015/04/17 12:59:33, rmcilroy wrote:
> > nit - I would prefer this function is inlined - i.e.: 
> >   bool gcFinished = false;
> >   if (deadlineSeconds > Platform::current()->monotonicallyIncreasingTime())
> >     gcFinished = isolate()->IdleNotificationDeadline(deadlineSeconds);
> > 
> >   if (gcFinished) {
> >     ...
> > 
> > I think this is a bit cleaner (and is similar to how V8Initilizer does it.
> WDYT?
> 
> I split the doIdleGc function off so I could test this.  Any idea how to test
it
> if I do what you suggest?

I missed you were using this in testing. In that case this is fine, but for
clarity let's change doIdleGc to:

  bool gcFinished = false;
  if (deadlineSeconds > Platform::current()->monotonicallyIncreasingTime())
    gcFinished = isolate()->IdleNotificationDeadline(deadlineSeconds);
  return gcFinished;

https://codereview.chromium.org/956333002/diff/340001/Source/core/workers/Wor...
File Source/core/workers/WorkerThreadTest.cpp (right):

https://codereview.chromium.org/956333002/diff/340001/Source/core/workers/Wor...
Source/core/workers/WorkerThreadTest.cpp:221: , m_taskCount(taskCount)
Could we make the public constructor not take 'taskCount' as an argument and add
a private constructor which does (it took me a while to work out why the test
below was passing '0' for this argument).

https://codereview.chromium.org/956333002/diff/340001/Source/core/workers/Wor...
Source/core/workers/WorkerThreadTest.cpp:303: TEST_F(WorkerThreadTest,
ShortGcDeadline)
Could we add one more test which ensures that if there is a task which is
scheduled to run <50ms after the start of the idle period that the idle deadline
is <50ms (i.e., only the remaining time until the other task is scheduled to
run).

https://codereview.chromium.org/956333002/diff/340001/Source/core/workers/Wor...
Source/core/workers/WorkerThreadTest.cpp:329: // The deadlin should be short if
there's a task that needs to run shortly after the idle period starts.
/s/deadlin/deadline

[commit-bot: I haz the power, 2015-04-17 16:38:25]

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

[commit-bot: I haz the power, 2015-04-17 16:38:27]

Dry run: Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[alex clarke (OOO till 29th), 2015-04-17 16:47:58]

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

[alex clarke (OOO till 29th), 2015-04-17 16:47:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/956333002/#ps360001
(title: "Fix compilation of TimerTest.cpp on gcc")

[commit-bot: I haz the power, 2015-04-17 16:48:10]

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

[commit-bot: I haz the power, 2015-04-17 17:32:42]

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

[commit-bot: I haz the power, 2015-04-17 17:32:45]

Dry run: Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[alex clarke (OOO till 29th), 2015-04-20 09:48:34]

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

[alex clarke (OOO till 29th), 2015-04-20 09:48:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/956333002/#ps380001
(title: "Fix componant build")

[commit-bot: I haz the power, 2015-04-20 09:49:22]

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

[alex clarke (OOO till 29th), 2015-04-20 10:06:54]

https://codereview.chromium.org/956333002/diff/320001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/956333002/diff/320001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:467: if (doIdleGc(gcDeadlineSeconds))
On 2015/04/17 16:33:22, rmcilroy wrote:
> On 2015/04/17 13:37:08, alexclarke1 wrote:
> > On 2015/04/17 12:59:33, rmcilroy wrote:
> > > nit - I would prefer this function is inlined - i.e.: 
> > >   bool gcFinished = false;
> > >   if (deadlineSeconds >
Platform::current()->monotonicallyIncreasingTime())
> > >     gcFinished = isolate()->IdleNotificationDeadline(deadlineSeconds);
> > > 
> > >   if (gcFinished) {
> > >     ...
> > > 
> > > I think this is a bit cleaner (and is similar to how V8Initilizer does it.
> > WDYT?
> > 
> > I split the doIdleGc function off so I could test this.  Any idea how to
test
> it
> > if I do what you suggest?
> 
> I missed you were using this in testing. In that case this is fine, but for
> clarity let's change doIdleGc to:
> 
>   bool gcFinished = false;
>   if (deadlineSeconds > Platform::current()->monotonicallyIncreasingTime())
>     gcFinished = isolate()->IdleNotificationDeadline(deadlineSeconds);
>   return gcFinished;

OK although I may get asked to change it back since I got the impression that
blink style prefers early return where possible.

https://codereview.chromium.org/956333002/diff/340001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/956333002/diff/340001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:253: RawPtr<WorkerThread> m_thread;
On 2015/04/17 15:34:31, Sami wrote:
> // Not owned?

Done.

https://codereview.chromium.org/956333002/diff/340001/Source/core/workers/Wor...
File Source/core/workers/WorkerThreadTest.cpp (right):

https://codereview.chromium.org/956333002/diff/340001/Source/core/workers/Wor...
Source/core/workers/WorkerThreadTest.cpp:303: TEST_F(WorkerThreadTest,
ShortGcDeadline)
On 2015/04/17 16:33:22, rmcilroy wrote:
> Could we add one more test which ensures that if there is a task which is
> scheduled to run <50ms after the start of the idle period that the idle
deadline
> is <50ms (i.e., only the remaining time until the other task is scheduled to
> run).

Done but I realized this throws up an interesting issue: If there is a delayed
task scheduled for some time during the next idle period,
canExceedIdleDeadlineIfRequired returns true and we get a 1s gc deadline.  Is
this desirable?

https://codereview.chromium.org/956333002/diff/340001/Source/core/workers/Wor...
Source/core/workers/WorkerThreadTest.cpp:329: // The deadlin should be short if
there's a task that needs to run shortly after the idle period starts.
On 2015/04/17 16:33:22, rmcilroy wrote:
> /s/deadlin/deadline

Done.

[alex clarke (OOO till 29th), 2015-04-20 15:56:58]

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

[alex clarke (OOO till 29th), 2015-04-20 15:56:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/956333002/#ps440001
(title: "Fix ScrollableAreaTest on mac")

[commit-bot: I haz the power, 2015-04-20 15:57:17]

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

[rmcilroy, 2015-04-20 20:56:50]

https://codereview.chromium.org/956333002/diff/320001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/956333002/diff/320001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:467: if (doIdleGc(gcDeadlineSeconds))
On 2015/04/20 10:06:53, alexclarke1 wrote:
> On 2015/04/17 16:33:22, rmcilroy wrote:
> > On 2015/04/17 13:37:08, alexclarke1 wrote:
> > > On 2015/04/17 12:59:33, rmcilroy wrote:
> > > > nit - I would prefer this function is inlined - i.e.: 
> > > >   bool gcFinished = false;
> > > >   if (deadlineSeconds >
> Platform::current()->monotonicallyIncreasingTime())
> > > >     gcFinished = isolate()->IdleNotificationDeadline(deadlineSeconds);
> > > > 
> > > >   if (gcFinished) {
> > > >     ...
> > > > 
> > > > I think this is a bit cleaner (and is similar to how V8Initilizer does
it.
> > > WDYT?
> > > 
> > > I split the doIdleGc function off so I could test this.  Any idea how to
> test
> > it
> > > if I do what you suggest?
> > 
> > I missed you were using this in testing. In that case this is fine, but for
> > clarity let's change doIdleGc to:
> > 
> >   bool gcFinished = false;
> >   if (deadlineSeconds > Platform::current()->monotonicallyIncreasingTime())
> >     gcFinished = isolate()->IdleNotificationDeadline(deadlineSeconds);
> >   return gcFinished;
> 
> OK 

Thanks!

> although I may get asked to change it back since I got the impression that
> blink style prefers early return where possible.

My interpretation of that rule is Blink style prefers early returns for 'early
outs' (e.g., if (x==NULL) return), however this was an early return for the
expected case. I may be wrong in that interpretation though.

In any case I think this is much clearer since it makes clear what the return
value of idleNotification represents, which I don't think is clear otherwise.

https://codereview.chromium.org/956333002/diff/340001/Source/core/workers/Wor...
File Source/core/workers/WorkerThreadTest.cpp (right):

https://codereview.chromium.org/956333002/diff/340001/Source/core/workers/Wor...
Source/core/workers/WorkerThreadTest.cpp:221: , m_taskCount(taskCount)
On 2015/04/17 16:33:22, rmcilroy wrote:
> Could we make the public constructor not take 'taskCount' as an argument and
add
> a private constructor which does (it took me a while to work out why the test
> below was passing '0' for this argument).

Still to do?

https://codereview.chromium.org/956333002/diff/340001/Source/core/workers/Wor...
Source/core/workers/WorkerThreadTest.cpp:303: TEST_F(WorkerThreadTest,
ShortGcDeadline)
On 2015/04/20 10:06:54, alexclarke1 wrote:
> On 2015/04/17 16:33:22, rmcilroy wrote:
> > Could we add one more test which ensures that if there is a task which is
> > scheduled to run <50ms after the start of the idle period that the idle
> deadline
> > is <50ms (i.e., only the remaining time until the other task is scheduled to
> > run).
> 
> Done but I realized this throws up an interesting issue: If there is a delayed
> task scheduled for some time during the next idle period,
> canExceedIdleDeadlineIfRequired returns true and we get a 1s gc deadline.  Is
> this desirable?

CanExceedIdleDeadlineIfRequired should only return true if there is no pending
task scheduled within the next 50ms idle period (that's what's implementented by
the SchedulerHelper anyway).

[commit-bot: I haz the power, 2015-04-20 21:08:28]

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

[commit-bot: I haz the power, 2015-04-20 21:08:30]

Dry run: Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/52592)

[alex clarke (OOO till 29th), 2015-04-23 12:49:18]

Patchset #7 (id:140001) has been deleted

[alex clarke (OOO till 29th), 2015-04-23 12:49:37]

Patchset #8 (id:180001) has been deleted

[Sami, 2015-05-07 15:57:27]

FYI, I moved this to https://codereview.chromium.org/1134523002.