Prioritizing input and compositor tasks in the blink scheduler.

Source/platform/scheduler/Scheduler.h

Index: Source/platform/scheduler/Scheduler.h
diff --git a/Source/platform/scheduler/Scheduler.h b/Source/platform/scheduler/Scheduler.h
index c492e580aa17268afe30b6c439a9c08acd08638f..23428efab2b11ebf60e87df3bc32abc03fa0591d 100644
--- a/Source/platform/scheduler/Scheduler.h
+++ b/Source/platform/scheduler/Scheduler.h
@@ -6,8 +6,12 @@
 #define Scheduler_h
 
 #include "platform/PlatformExport.h"
+#include "platform/TraceLocation.h"
+#include "public/platform/WebThread.h"
+#include "wtf/Deque.h"
 #include "wtf/Functional.h"
 #include "wtf/Noncopyable.h"
+#include "wtf/ThreadingPrimitives.h"
 
 namespace blink {
 class WebThread;
@@ -53,16 +57,54 @@ private:
     Scheduler();
     ~Scheduler();
 
-    void scheduleTask(const TraceLocation&, const Task&);
     void scheduleIdleTask(const IdleTask&);
 
     static void sharedTimerAdapter();
     void tickSharedTimer();
 
     static Scheduler* s_sharedScheduler;
-    blink::WebThread* m_mainThread;
 
+    class TracedTask {

[eseidel, 2014/08/07 17:18:15]

Why aren't all tasks's traced?  I think we want one task type in all of Blink
that just has all teh bells and whistles.

[Sami, 2014/08/07 18:22:22]

This is just an internal helper class, but I guess we could promote it to an
actual class to scheduler/Task.h (replacing the typedef here) and make sure it
always has an associated TraceLocation. That would also fix the bug that idle
tasks currently have no TraceLocation.

[alexclarke, 2014/08/08 13:32:23]

I'm not sure if you're suggesting we make TracedTask public or not.

We could make it public, but it would mean more typing for the clients of this
class, and the postTask methods would have a different looking interface than
the chromium postTask.

What do you think?

[Sami, 2014/08/08 15:31:12]

I think I agree that matching base's PostTask() interface is probably the better
choice and results in much less typing at the call site. We should fix idle
tasks though (in a different CL).

+    public:
+        TracedTask(const Task& task, const TraceLocation& location)
+            : m_task(task)
+            , m_location(location) { }
+
+        void run();
+
+    private:
+        Task m_task;
+        TraceLocation m_location;
+    };
+
+    class LockingTracedTaskDeque {
+    public:
+        void append(const TracedTask& value);
+        bool isEmpty();
+
+        // Returns true if a task was executed, false otherwise.
+        bool runFirstTaskIfNotEmpty();
+
+    private:
+        WTF::Deque<TracedTask> m_queue;
+        Mutex m_mutex;
+    };
+
+    class MainThreadPendingTaskRunner;
+
+    // These members can be accessed from any thread.
+    blink::WebThread* m_mainThread;
+    LockingTracedTaskDeque m_pendingInputTasks;

[eseidel, 2014/08/07 17:18:15]

Do these want separate locks or one big global lock?  This feels like we're
re-inventing MessageQueue from chromium?

Chromium avoids locks on each trip through the message loop by not locking on
the main queue and only locking when swapping the pending and active queues.

[Sami, 2014/08/07 18:22:22]

I guess a global lock would be better if there is little contention, which I
believe is the case here, because input and compositing tasks are generated
during different parts of the vsync cycle.
I like the double buffered queue idea.

[alexclarke, 2014/08/08 13:32:23]

Double buffering the queues is an interesting idea.   I'm quite keen to avoid
locks if I could but to make it properly thread safe, the incoming queue needs
locks on append and for the swap.  I notice that's what chromium's
IncomingTaskQueue does.

The advantage to double buffering is pulling tasks off the non-incoming queue no
longer needs locks. So I think I'll try that, although ideally it would be nice
to use a lockfree deque.  Boost has one but I don't know if it works on all
platforms.

+    LockingTracedTaskDeque m_pendingCompositorTasks;
+    LockingTracedTaskDeque m_pendingLowPriorityTasks;
     void (*m_sharedTimerFunction)();
+    volatile int m_mainThreadTaskRunnerCount;

[eseidel, 2014/08/07 17:18:15]

volatile?  I've never seen this used in Blink.  I presume this makes compilers
not be able to cache the value on the cpu, etc?

[Sami, 2014/08/07 18:23:59]

Yes, it's needed for atomic operations (and also used by wtf/Atomics.h). Again
base has a typedef to hide this ugliness which I think we should adopt.

[alexclarke, 2014/08/08 13:32:23]

Pretty much.  It's used in a few places in blink: 
https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ...

It's worth noting that volatile by itself does not imply thread safety, you need
atomics, memory barriers or locks in addition.

+
+    void maybePostMainThreadPendingTaskRunner();
+    void runHighPriorityTasks();
+    void runPendingTasks();
+
+    friend class MainThreadPendingTaskRunner;
 };
 
 } // namespace blink