Prioritizing input and compositor tasks in the blink scheduler.

Source/platform/scheduler/Scheduler.cpp

Index: Source/platform/scheduler/Scheduler.cpp
diff --git a/Source/platform/scheduler/Scheduler.cpp b/Source/platform/scheduler/Scheduler.cpp
index 61ae7aef1d486bd618417a2d2d62c14c08db06bc..b63f155a701e2c5f2bc78872eb56e0599dcfffaf 100644
--- a/Source/platform/scheduler/Scheduler.cpp
+++ b/Source/platform/scheduler/Scheduler.cpp
@@ -5,60 +5,61 @@
 #include "config.h"
 #include "platform/scheduler/Scheduler.h"
 
+#include "platform/PlatformThreadData.h"
 #include "platform/Task.h"
+#include "platform/ThreadTimers.h"
 #include "platform/TraceEvent.h"
-#include "platform/TraceLocation.h"
 #include "public/platform/Platform.h"
-#include "public/platform/WebThread.h"
+#include "wtf/ThreadingPrimitives.h"
 
 namespace blink {
 
 namespace {
 
-class MainThreadTaskAdapter : public blink::WebThread::Task {
+class MainThreadIdleTaskAdapter : public blink::WebThread::Task {
 public:
-    explicit MainThreadTaskAdapter(const TraceLocation& location, const Scheduler::Task& task)
-        : m_location(location)
-        , m_task(task)
+    MainThreadIdleTaskAdapter(const Scheduler::IdleTask& idleTask, double allottedTimeMs)
+        : m_idleTask(idleTask)
+        , m_allottedTimeMs(allottedTimeMs)
     {
     }
 
     // WebThread::Task implementation.
     virtual void run() OVERRIDE
     {
-        TRACE_EVENT2("blink", "MainThreadTaskAdapter::run",
-            "src_file", m_location.fileName(),
-            "src_func", m_location.functionName());
-        m_task();
+        TRACE_EVENT1("blink", "MainThreadIdleTaskAdapter::run", "allottedTime", m_allottedTimeMs);
+        m_idleTask(m_allottedTimeMs);
     }
 
 private:
-    const TraceLocation m_location;
-    Scheduler::Task m_task;
+    Scheduler::IdleTask m_idleTask;
+    double m_allottedTimeMs;
 };
 
-class MainThreadIdleTaskAdapter : public blink::WebThread::Task {
+} // namespace
+
+class Scheduler::MainThreadPendingTaskRunner : public blink::WebThread::Task {
 public:
-    MainThreadIdleTaskAdapter(const Scheduler::IdleTask& idleTask, double allottedTimeMs)
-        : m_idleTask(idleTask)
-        , m_allottedTimeMs(allottedTimeMs)
+    MainThreadPendingTaskRunner(volatile int* taskRunnerCount)
+        : m_taskRunnerCount(taskRunnerCount)
     {
+        atomicIncrement(m_taskRunnerCount);
+    }
+
+    ~MainThreadPendingTaskRunner()
+    {
+        atomicDecrement(m_taskRunnerCount);
     }
 
     // WebThread::Task implementation.
     virtual void run() OVERRIDE
     {
-        TRACE_EVENT1("blink", "MainThreadIdleTaskAdapter::run", "allottedTime", m_allottedTimeMs);
-        m_idleTask(m_allottedTimeMs);
+        Scheduler::shared()->runPendingTasks();

[Sami, 2014/08/07 15:51:26]

It would be slightly neater to pass in the scheduler instance to this class
instead of relying on a global.

[alexclarke, 2014/08/07 16:01:29]

Done.

     }
 
-private:
-    Scheduler::IdleTask m_idleTask;
-    double m_allottedTimeMs;
+    volatile int* m_taskRunnerCount; // Not owned.
 };
 
-}
-
 Scheduler* Scheduler::s_sharedScheduler = nullptr;
 
 void Scheduler::initializeOnMainThread()
@@ -80,16 +81,14 @@ Scheduler* Scheduler::shared()
 Scheduler::Scheduler()
     : m_mainThread(blink::Platform::current()->currentThread())
     , m_sharedTimerFunction(nullptr)
+    , m_mainThreadInFlight(0)

[Sami, 2014/08/07 15:51:26]

Bikeshedding again: m_mainThreadTaskRunnerCount?

[alexclarke, 2014/08/07 16:01:29]

Done.

 {
 }
 
 Scheduler::~Scheduler()
 {
-}
-
-void Scheduler::scheduleTask(const TraceLocation& location, const Task& task)
-{
-    m_mainThread->postTask(new MainThreadTaskAdapter(location, task));
+    while (!m_pendingLowPriorityTasks.isEmpty())
+        runPendingTasks();
 }
 
 void Scheduler::scheduleIdleTask(const IdleTask& idleTask)
@@ -100,17 +99,20 @@ void Scheduler::scheduleIdleTask(const IdleTask& idleTask)
 
 void Scheduler::postTask(const TraceLocation& location, const Task& task)
 {
-    scheduleTask(location, task);
+    m_pendingLowPriorityTasks.append(TracedTask(task, location));
+    maybePostTaskLoopOnMainThread();
 }
 
 void Scheduler::postInputTask(const TraceLocation& location, const Task& task)
 {
-    scheduleTask(location, task);
+    m_pendingInputTasks.append(TracedTask(task, location));
+    maybePostTaskLoopOnMainThread();
 }
 
 void Scheduler::postCompositorTask(const TraceLocation& location, const Task& task)
 {
-    scheduleTask(location, task);
+    m_pendingCompositorTasks.append(TracedTask(task, location));
+    maybePostTaskLoopOnMainThread();
 }
 
 void Scheduler::postIdleTask(const IdleTask& idleTask)
@@ -121,7 +123,47 @@ void Scheduler::postIdleTask(const IdleTask& idleTask)
 void Scheduler::tickSharedTimer()
 {
     TRACE_EVENT0("blink", "Scheduler::tickSharedTimer");
+
+    runHighPriorityTasks();
     m_sharedTimerFunction();
+    runHighPriorityTasks();
+}
+
+void Scheduler::runPendingTasks()
+{
+    TRACE_EVENT0("blink", "Scheduler::runPendingTasks");
+
+    // Execute high priority tasks first.
+    runHighPriorityTasks();
+
+    if (!m_pendingLowPriorityTasks.runFirstTaskIfNotEmpty())
+        return;
+
+    // To avoid staving main loop tasks that don't (yet?) go through the scheduler, we only
+    // execute one low priority task rather than all of them.
+    if (!m_pendingLowPriorityTasks.isEmpty())
+        m_mainThread->postTask(new MainThreadPendingTaskRunner(&m_mainThreadInFlight));

[Sami, 2014/08/07 15:51:26]

Why not just call maybePostTaskLoopOnMainThread here?

[alexclarke, 2014/08/07 16:01:29]

Because the reference count is not zero.

+}
+
+void Scheduler::runHighPriorityTasks()
+{
+    TRACE_EVENT0("blink", "Scheduler::runHighPriorityTasks");
+
+    for (;;) {
+        if (m_pendingInputTasks.runFirstTaskIfNotEmpty())
+            continue;
+
+        if (!m_pendingCompositorTasks.runFirstTaskIfNotEmpty())
+            break;
+    }
+}
+
+void Scheduler::maybePostTaskLoopOnMainThread()

[Sami, 2014/08/07 15:51:26]

My bikeshed sense is tingling again :) I like having a single name for a single
thing, so how about maybePostMainThreadPendingTaskRunner()?

[alexclarke, 2014/08/07 16:01:29]

Done.

+{
+    if (acquireLoad(&m_mainThreadInFlight))
+        return;
+
+    m_mainThread->postTask(new MainThreadPendingTaskRunner(&m_mainThreadInFlight));

[Sami, 2014/08/07 15:51:26]

It's worth noting that this can cause several task runners to get posted. I
don't think that should cause any real problems though. Let's still add a
comment here.

[alexclarke, 2014/08/07 16:01:29]

Done.

 }
 
 void Scheduler::sharedTimerAdapter()
@@ -147,7 +189,41 @@ void Scheduler::stopSharedTimer()
 
 bool Scheduler::shouldYieldForHighPriorityWork()
 {
-    return false;
+    return !m_pendingInputTasks.isEmpty() || !m_pendingCompositorTasks.isEmpty();
+}
+
+void Scheduler::TracedTask::run()
+{
+    TRACE_EVENT2("blink", "TracedTask::run",
+        "src_file", m_location.fileName(),
+        "src_func", m_location.functionName());
+    m_task();
+}
+
+void Scheduler::LockingTracedTaskDeque::append(const TracedTask& value)
+{
+    Locker<Mutex> lock(m_mutex);
+    m_queue.append(value);
+}
+
+bool Scheduler::LockingTracedTaskDeque::isEmpty()
+{
+    Locker<Mutex> lock(m_mutex);
+    return m_queue.isEmpty();
+}
+
+bool Scheduler::LockingTracedTaskDeque::runFirstTaskIfNotEmpty()
+{
+    m_mutex.lock();
+    if (m_queue.isEmpty()) {
+        m_mutex.unlock();
+        return false;
+    }
+    TracedTask task = m_queue.takeFirst();
+    m_mutex.unlock();
+
+    task.run();
+    return true;
 }
 
 } // namespace blink