Use the scheduling mechanism provided by the platform

Use the scheduling mechanism provided by the platform

This uses the newly added WebScheduler interface for posting idle
tasks and checking whether main thread work should yield to let
more critical tasks run. This interface is implemented by the platform.

Design doc: https://docs.google.com/a/chromium.org/document/d/16f_RIhZa47uEK_OdtTgzWdRU0RFMTQWMpEWyWXIpXUo/edit#heading=h.srz53flt1rrp

BUG=391005
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184374

[rmcilroy, 2014-10-21 10:54:15]

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

[rmcilroy, 2014-10-21 10:54:16]

https://codereview.chromium.org/656463004/diff/130001/public/platform/WebSche...
File public/platform/WebScheduler.h (right):

https://codereview.chromium.org/656463004/diff/130001/public/platform/WebSche...
public/platform/WebScheduler.h:34: // Can be called on any thread.
This should only be called from the main thread now (due to pollQueue needing to
be called from the main thread).

[Sami, 2014-10-21 11:42:55]

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

[Sami, 2014-10-21 11:44:18]

I think this is now ready for review. PTAL.

https://codereview.chromium.org/656463004/diff/130001/public/platform/WebSche...
File public/platform/WebScheduler.h (right):

https://codereview.chromium.org/656463004/diff/130001/public/platform/WebSche...
public/platform/WebScheduler.h:34: // Can be called on any thread.
On 2014/10/21 10:54:15, rmcilroy wrote:
> This should only be called from the main thread now (due to pollQueue needing
to
> be called from the main thread).

Indeed, let's make it so.

[Sami, 2014-10-21 13:08:43]

Ross, I've now also changed v8 to use the new idle task API. PTAL.

[rmcilroy, 2014-10-21 13:24:19]

V8 changes and WebScheduler.h lgtm, thanks!

https://codereview.chromium.org/656463004/diff/170001/public/platform/WebSche...
File public/platform/WebScheduler.h (right):

https://codereview.chromium.org/656463004/diff/170001/public/platform/WebSche...
public/platform/WebScheduler.h:29: // scheduler. Must be alled on the main
thread.
/s/alled/called :)

[rmcilroy, 2014-10-21 13:24:19]

V8 changes and WebScheduler.h lgtm, thanks!

https://codereview.chromium.org/656463004/diff/170001/public/platform/WebSche...
File public/platform/WebScheduler.h (right):

https://codereview.chromium.org/656463004/diff/170001/public/platform/WebSche...
public/platform/WebScheduler.h:29: // scheduler. Must be alled on the main
thread.
/s/alled/called :)

[Sami, 2014-10-21 13:54:12]

https://codereview.chromium.org/656463004/diff/170001/public/platform/WebSche...
File public/platform/WebScheduler.h (right):

https://codereview.chromium.org/656463004/diff/170001/public/platform/WebSche...
public/platform/WebScheduler.h:29: // scheduler. Must be alled on the main
thread.
On 2014/10/21 13:24:19, rmcilroy wrote:
> /s/alled/called :)

Yeah, sounds better that way :)

[eseidel, 2014-10-21 16:11:02]

https://codereview.chromium.org/656463004/diff/190002/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/656463004/diff/190002/Source/bindings/core/v8...
Source/bindings/core/v8/V8Initializer.cpp:313: postIdleGCTaskMainThread();
So we'll always have a GC task in the run loop?  Won't that just churn battery
waking up the machine on an idle page?

I see that was already the case, but it seems bad.

https://codereview.chromium.org/656463004/diff/190002/Source/bindings/core/v8...
Source/bindings/core/v8/V8Initializer.cpp:319: if
(!RuntimeEnabledFeatures::v8IdleTasksEnabled())
Previously this was an assert?

https://codereview.chromium.org/656463004/diff/190002/Source/bindings/core/v8...
Source/bindings/core/v8/V8Initializer.cpp:321: if (WebScheduler* scheduler =
Platform::current()->scheduler())
When do we ever have a platform but not have a scheduler?

https://codereview.chromium.org/656463004/diff/190002/Source/bindings/core/v8...
Source/bindings/core/v8/V8Initializer.cpp:322:
scheduler->postIdleTask(FROM_HERE.toWebTraceLocation(), new
IdleGCTaskInMainThread());
We don't just have a shared task which we reuse?  Does the scheduler delete our
tasks for us?

https://codereview.chromium.org/656463004/diff/190002/Source/platform/ThreadT...
File Source/platform/ThreadTimers.cpp (right):

https://codereview.chromium.org/656463004/diff/190002/Source/platform/ThreadT...
Source/platform/ThreadTimers.cpp:145: if (isMainThread() &&
Platform::current()->scheduler() &&
Platform::current()->scheduler()->shouldYieldForHighPriorityWork())
When does this ever fire not on the main thread?  WE could just ASSERT that.

https://codereview.chromium.org/656463004/diff/190002/Source/web/WebKit.cpp
File Source/web/WebKit.cpp (right):

https://codereview.chromium.org/656463004/diff/190002/Source/web/WebKit.cpp#n...
Source/web/WebKit.cpp:228: if (WebScheduler* scheduler =
Platform::current()->scheduler())
Does this need to be before WTF and Heap?  Is this just draining the event
queue?

https://codereview.chromium.org/656463004/diff/190002/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/656463004/diff/190002/public/platform/Platfor...
public/platform/Platform.h:364: // May return null.
I think instead of letting this return null and needing to check null everywhere
we should consider just having a dummy scheduler like we do for EmptyClients.

https://codereview.chromium.org/656463004/diff/190002/public/platform/WebSche...
File public/platform/WebScheduler.h (right):

https://codereview.chromium.org/656463004/diff/190002/public/platform/WebSche...
public/platform/WebScheduler.h:30: virtual void shutdown() { }
Why are these {} instead of = 0?

https://codereview.chromium.org/656463004/diff/190002/public/platform/WebSche...
public/platform/WebScheduler.h:40: // Takes ownership of |IdleTask|. Can be
called from any thread.
Which allocator does IdleTask use?  I guess the Web-API isn't already memory
tight to begin with... but it seems slightly strange that we might alloc
IdleTask with one allocator and try to destroy it with another.

[Sami, 2014-10-21 18:49:35]

Thanks for the comments Eric. I've rewritten this to keep the platform-level
helper so that we don't have to check for the existence of the platform
scheduler everywhere.

https://codereview.chromium.org/656463004/diff/190002/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/656463004/diff/190002/Source/bindings/core/v8...
Source/bindings/core/v8/V8Initializer.cpp:313: postIdleGCTaskMainThread();
On 2014/10/21 16:11:02, eseidel wrote:
> So we'll always have a GC task in the run loop?  Won't that just churn battery
> waking up the machine on an idle page?
> 
> I see that was already the case, but it seems bad.

The idle queue is disabled while the compositor isn't actively drawing anything,
so this won't actually cause any extra wake-ups or spinning. The FIXME here is
about making this smarter about only posting a task when necessary, but right
now it won't make a measurable difference either way.

https://codereview.chromium.org/656463004/diff/190002/Source/bindings/core/v8...
Source/bindings/core/v8/V8Initializer.cpp:319: if
(!RuntimeEnabledFeatures::v8IdleTasksEnabled())
On 2014/10/21 16:11:02, eseidel wrote:
> Previously this was an assert?

Sorry, the diff is a little confusing since I moved from code below this
function to above it. I just changed this particular check to an early-out.

https://codereview.chromium.org/656463004/diff/190002/Source/bindings/core/v8...
Source/bindings/core/v8/V8Initializer.cpp:321: if (WebScheduler* scheduler =
Platform::current()->scheduler())
On 2014/10/21 16:11:02, eseidel wrote:
> When do we ever have a platform but not have a scheduler?

Initially I thought we would need this for the bootstrap phase, but now that
I've kept the helper in platform/scheduler/Scheduler.h we'll always have a
fallback.

https://codereview.chromium.org/656463004/diff/190002/Source/bindings/core/v8...
Source/bindings/core/v8/V8Initializer.cpp:322:
scheduler->postIdleTask(FROM_HERE.toWebTraceLocation(), new
IdleGCTaskInMainThread());
On 2014/10/21 16:11:02, eseidel wrote:
> We don't just have a shared task which we reuse?  Does the scheduler delete
our
> tasks for us?

Yes, that's the best we can do with the current model since we can't share
closures across the repo boundary :( The ownership needs to be on the scheduler
side because here we don't know when anything is actually run. That means we
need to allocate a new object every time.

Note that we already need to do this for everything that goes through
WebThread::postTask().

An alternative would be having a version that takes just a function pointer and
an argument, but since you can't easily wrap that into a closure on the Blink
side, we prefer to pass an actual object through this interface instead.

https://codereview.chromium.org/656463004/diff/190002/Source/platform/ThreadT...
File Source/platform/ThreadTimers.cpp (right):

https://codereview.chromium.org/656463004/diff/190002/Source/platform/ThreadT...
Source/platform/ThreadTimers.cpp:145: if (isMainThread() &&
Platform::current()->scheduler() &&
Platform::current()->scheduler()->shouldYieldForHighPriorityWork())
On 2014/10/21 16:11:02, eseidel wrote:
> When does this ever fire not on the main thread?  WE could just ASSERT that.

Web workers can have timers too.

https://codereview.chromium.org/656463004/diff/190002/Source/web/WebKit.cpp
File Source/web/WebKit.cpp (right):

https://codereview.chromium.org/656463004/diff/190002/Source/web/WebKit.cpp#n...
Source/web/WebKit.cpp:228: if (WebScheduler* scheduler =
Platform::current()->scheduler())
On 2014/10/21 16:11:02, eseidel wrote:
> Does this need to be before WTF and Heap?  Is this just draining the event
> queue?

Yes, it needs to be right between CoreInitializer::shutdown() (which can still
post new tasks, e.g., from JS) and Heap::shutdown() (which makes any posted
tasks invalid -- we can't even delete them safely after that point). Shutdown
deletes all the pending tasks we have queued up but doesn't run them.

https://codereview.chromium.org/656463004/diff/190002/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/656463004/diff/190002/public/platform/Platfor...
public/platform/Platform.h:364: // May return null.
On 2014/10/21 16:11:02, eseidel wrote:
> I think instead of letting this return null and needing to check null
everywhere
> we should consider just having a dummy scheduler like we do for EmptyClients.

That's a great idea. Since it doesn't look like we can easily return anything
non-null from here, I've chosen to keep a helper in
platform/scheduler/Scheduler.h which will transparently fall back if the
platform doesn't provide a real scheduler (which is the case when we first land
this patch).

The helper also makes sense because we can translate between Blink and Web types
there.

https://codereview.chromium.org/656463004/diff/190002/public/platform/WebSche...
File public/platform/WebScheduler.h (right):

https://codereview.chromium.org/656463004/diff/190002/public/platform/WebSche...
public/platform/WebScheduler.h:30: virtual void shutdown() { }
On 2014/10/21 16:11:02, eseidel wrote:
> Why are these {} instead of = 0?

This is the convention for when the implementation lives in a separate
repository. If these were = 0, then this patch couldn't land because the
implementation isn't there yet, because it depends on this patch :) Also,
changing any of the functions becomes super difficult if you use = 0 (or
override on the other side).

https://codereview.chromium.org/656463004/diff/190002/public/platform/WebSche...
public/platform/WebScheduler.h:40: // Takes ownership of |IdleTask|. Can be
called from any thread.
On 2014/10/21 16:11:02, eseidel wrote:
> Which allocator does IdleTask use?  I guess the Web-API isn't already memory
> tight to begin with... but it seems slightly strange that we might alloc
> IdleTask with one allocator and try to destroy it with another.

I don't think we'll end up doing that. |new| and |delete| use the same heap on
both sides because we use the same C++ runtime, so everything happens in the
same heap by default. Any potential problems come from objects owned by the task
which use different heaps (such as RefPtr, which uses PartitionAlloc). We handle
that by making sure no tasks remain when PartitionAlloc shuts down.

[Sami, 2014-10-22 12:57:20]

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

[Sami, 2014-10-24 09:55:02]

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

[Sami, 2014-10-24 09:56:30]

Hey Jochen, could you take a look here? This basically adds a platform interface
(WebScheduler) for implementing scheduling and deletes all the old stuff we no
longer need.

[jochen (gone - plz use gerrit), 2014-10-24 11:42:12]

can you put adding the new interface and deleting the old implementation into
two separate CLs?

https://codereview.chromium.org/656463004/diff/250001/public/platform/WebSche...
File public/platform/WebScheduler.h (right):

https://codereview.chromium.org/656463004/diff/250001/public/platform/WebSche...
public/platform/WebScheduler.h:30: virtual void shutdown() { }
nit, why not make them all pure virtual?

[Sami, 2014-10-24 12:01:33]

> can you put adding the new interface and deleting the old 
> implementation into two separate CLs?

Certainly! I've pulled the interface into this one:
https://codereview.chromium.org/656323005/

https://codereview.chromium.org/656463004/diff/250001/public/platform/WebSche...
File public/platform/WebScheduler.h (right):

https://codereview.chromium.org/656463004/diff/250001/public/platform/WebSche...
public/platform/WebScheduler.h:30: virtual void shutdown() { }
On 2014/10/24 11:42:12, jochen (OOO real soon now) wrote:
> nit, why not make them all pure virtual?

Simply because the implementation lives in Chromium and if you make things that
cross repo boundaries pure virtual, you're gonna have a bad time. Many of the
other platform interfaces use this convention. Once the repos merge we can be
more strict.

[Sami, 2014-10-24 13:43:17]

skyostil@chromium.org changed reviewers:
+ mkwst@chromium.org
- eseidel@chromium.org, jochen@chromium.org

[Sami, 2014-10-24 13:48:57]

Hi Mike. Jochen suggested you might be able to help us with this patch. It's
basically removing the old in-Blink scheduler implementation and delegating the
remaining bits to the WebScheduler that comes from the platform.

[Mike West, 2014-10-24 14:59:35]

LGTM % a few things.

1. The design doc doesn't open for me in an incognito window. Could you please
ensure that it's world-readable?

2. This CL should be linked to the same bug as the other CL. It would probably
have been reasonable to create a sub-bug for the scheduler-specific work,
actually, as the bug that's linked from the other CL is a bit generic. :)

https://codereview.chromium.org/656463004/diff/250001/Source/platform/TraceLo...
File Source/platform/TraceLocation.h (right):

https://codereview.chromium.org/656463004/diff/250001/Source/platform/TraceLo...
Source/platform/TraceLocation.h:16: class TraceLocation {
Hrm. It seems like TraceLocation should have an explicit relationship with
WebTraceLocation. Could you implement WebTraceLocation as a wrapper around
TraceLocation rather than creating new objects when casting around?

Probably work for a separate CL, of course.

https://codereview.chromium.org/656463004/diff/270001/Source/platform/TraceLo...
File Source/platform/TraceLocation.h (right):

https://codereview.chromium.org/656463004/diff/270001/Source/platform/TraceLo...
Source/platform/TraceLocation.h:18: // Currenetly only store the bits used in
Blink, base::Location stores more.
Nit: s/Currenetly/Currently/g

[Sami, 2014-10-24 15:24:39]

Thanks Mike!

> 1. The design doc doesn't open for me in an incognito window. Could you please
> ensure that it's world-readable?

Oops, fixed. It was @chromium.org only previously but now it's open for
everyone.

> 2. This CL should be linked to the same bug as the other CL. It would probably
> have been reasonable to create a sub-bug for the scheduler-specific work,
> actually, as the bug that's linked from the other CL is a bit generic. :)

Agreed, we've been using crbug.com/391005 as a grab-bag for this stage of the
scheduling work, but should defitely start using more targeted bugs.

>
https://codereview.chromium.org/656463004/diff/250001/Source/platform/TraceLo...
> Source/platform/TraceLocation.h:16: class TraceLocation {
> Hrm. It seems like TraceLocation should have an explicit relationship with
> WebTraceLocation. Could you implement WebTraceLocation as a wrapper around
> TraceLocation rather than creating new objects when casting around?
> 
> Probably work for a separate CL, of course.

Good idea, I'll whip up a CL to do that.

https://codereview.chromium.org/656463004/diff/270001/Source/platform/TraceLo...
File Source/platform/TraceLocation.h (right):

https://codereview.chromium.org/656463004/diff/270001/Source/platform/TraceLo...
Source/platform/TraceLocation.h:18: // Currenetly only store the bits used in
Blink, base::Location stores more.
On 2014/10/24 14:59:35, Mike West wrote:
> Nit: s/Currenetly/Currently/g

Done.

[Sami, 2014-10-24 15:26:28]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-10-24 15:27:03]

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

[commit-bot: I haz the power, 2014-10-24 17:51:49]

Committed patchset #13 (id:290001) as 184374