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..b5dc150d0e425dfb7dc5b02c0e96e389c8c83f38 100644
--- a/Source/platform/scheduler/Scheduler.cpp
+++ b/Source/platform/scheduler/Scheduler.cpp
@@ -5,59 +5,103 @@
 #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 {
+// Can be created from any thread.
+// Note if the scheduler gets shutdown, this may be run after.
+class MainThreadIdleTaskAdapter : public 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, const TraceLocation& location)
+        : m_idleTask(idleTask)
+        , m_allottedTimeMs(allottedTimeMs)
+        , m_location(location)
     {
     }
 
     // WebThread::Task implementation.
     virtual void run() OVERRIDE
     {
-        TRACE_EVENT2("blink", "MainThreadTaskAdapter::run",
+        TRACE_EVENT2("blink", "MainThreadIdleTaskAdapter::run",
             "src_file", m_location.fileName(),
             "src_func", m_location.functionName());
-        m_task();
+        m_idleTask(m_allottedTimeMs);
     }
 
 private:
-    const TraceLocation m_location;
-    Scheduler::Task m_task;
+    Scheduler::IdleTask m_idleTask;
+    double m_allottedTimeMs;
+    TraceLocation m_location;
 };
 
-class MainThreadIdleTaskAdapter : public blink::WebThread::Task {
+} // namespace
+
+// Typically only created from compositor or render threads.
+// Note if the scheduler gets shutdown, this may be run after.
+class Scheduler::MainThreadPendingHighPriorityTaskRunner : public WebThread::Task {
 public:
-    MainThreadIdleTaskAdapter(const Scheduler::IdleTask& idleTask, double allottedTimeMs)
-        : m_idleTask(idleTask)
-        , m_allottedTimeMs(allottedTimeMs)
+    MainThreadPendingHighPriorityTaskRunner()
+    {
+        ASSERT(Scheduler::shared());
+    }
+
+    ~MainThreadPendingHighPriorityTaskRunner()
     {
+        Scheduler* scheduler = Scheduler::shared();

[Sami, 2014/08/12 13:02:59]

Looks like the destructor should be empty here?

[alexclarke, 2014/08/12 13:59:28]

Done.

+        if (!scheduler)
+            return;
     }
 
     // WebThread::Task implementation.
     virtual void run() OVERRIDE
     {
-        TRACE_EVENT1("blink", "MainThreadIdleTaskAdapter::run", "allottedTime", m_allottedTimeMs);
-        m_idleTask(m_allottedTimeMs);
+        Scheduler* scheduler = Scheduler::shared();
+        if (!scheduler)
+            return;
+        scheduler->runHighPriorityTasks();
     }
-
-private:
-    Scheduler::IdleTask m_idleTask;
-    double m_allottedTimeMs;
 };
 
-}
+
+// Can be created from any thread.
+// Note if the scheduler gets shutdown, this may be run after.
+class Scheduler::MainThreadPendingTaskRunner : public WebThread::Task {
+public:
+    MainThreadPendingTaskRunner(
+        const Scheduler::Task& task, const TraceLocation& location)
+        : m_task(task, location)
+    {
+        ASSERT(Scheduler::shared());
+    }
+
+    ~MainThreadPendingTaskRunner()
+    {
+        Scheduler* scheduler = Scheduler::shared();

[Sami, 2014/08/12 13:02:59]

Same here?

[alexclarke, 2014/08/12 13:59:28]

Done.

+        if (!scheduler)
+            return;
+    }
+
+    // WebThread::Task implementation.
+    virtual void run() OVERRIDE
+    {
+        Scheduler* scheduler = Scheduler::shared();
+        // This will assert if a posted task outlives blink, which is probably a bug.
+        ASSERT(scheduler);
+        if (scheduler)
+            Scheduler::shared()->runHighPriorityTasks();
+        m_task.run();
+    }
+
+    Scheduler::TracedTask m_task;
+};
 
 Scheduler* Scheduler::s_sharedScheduler = nullptr;
 
@@ -80,48 +124,85 @@ Scheduler* Scheduler::shared()
 Scheduler::Scheduler()
     : m_mainThread(blink::Platform::current()->currentThread())
     , m_sharedTimerFunction(nullptr)
+    , m_highPriotityTaskCount(0)
 {
 }
 
 Scheduler::~Scheduler()
 {
+    while (shouldYieldForHighPriorityWork()) {

[Sami, 2014/08/12 13:02:59]

I'm slightly worried that shouldYieldForHighPriorityWork will end up having more
complex logic and will end up breaking this function. Let's add an internal
hasPendingHighPriorityWork() helper instead and have
shouldYieldForHighPriorityWork() call that.

[alexclarke, 2014/08/12 13:59:28]

Done.

+        runHighPriorityTasks();
+    }
 }
 
-void Scheduler::scheduleTask(const TraceLocation& location, const Task& task)
-{
-    m_mainThread->postTask(new MainThreadTaskAdapter(location, task));
-}
-
-void Scheduler::scheduleIdleTask(const IdleTask& idleTask)
+void Scheduler::scheduleIdleTask(const TraceLocation& location, const IdleTask& idleTask)
 {
     // TODO: send a real allottedTime here.
-    m_mainThread->postTask(new MainThreadIdleTaskAdapter(idleTask, 0));
+    m_mainThread->postTask(new MainThreadIdleTaskAdapter(idleTask, 0, location));
 }
 
 void Scheduler::postTask(const TraceLocation& location, const Task& task)
 {
-    scheduleTask(location, task);
+    m_mainThread->postTask(new MainThreadPendingTaskRunner(task, location));
 }
 
 void Scheduler::postInputTask(const TraceLocation& location, const Task& task)
 {
-    scheduleTask(location, task);
+    Locker<Mutex> lock(m_pendingTasksMutex);
+    m_pendingInputTasks.append(TracedTask(task, location));
+    atomicIncrement(&m_highPriotityTaskCount);
+    m_mainThread->postTask(new MainThreadPendingHighPriorityTaskRunner());
 }
 
 void Scheduler::postCompositorTask(const TraceLocation& location, const Task& task)
 {
-    scheduleTask(location, task);
+    Locker<Mutex> lock(m_pendingTasksMutex);
+    m_pendingCompositorTasks.append(TracedTask(task, location));
+    atomicIncrement(&m_highPriotityTaskCount);
+    m_mainThread->postTask(new MainThreadPendingHighPriorityTaskRunner());
 }
 
-void Scheduler::postIdleTask(const IdleTask& idleTask)
+void Scheduler::postIdleTask(const TraceLocation& location, const IdleTask& idleTask)
 {
-    scheduleIdleTask(idleTask);
+    scheduleIdleTask(location, idleTask);
 }
 
 void Scheduler::tickSharedTimer()
 {
     TRACE_EVENT0("blink", "Scheduler::tickSharedTimer");
+
+    // Run any high priority tasks that are queued up, otherwise the blink timers will yield immediately.
+    runHighPriorityTasks();
     m_sharedTimerFunction();
+
+    // The blink timers may have just yielded, so run any high priority tasks that where queued up
+    // while the blink timers were executing.
+    runHighPriorityTasks();
+}
+
+void Scheduler::runHighPriorityTasks()
+{
+    TRACE_EVENT0("blink", "Scheduler::runHighPriorityTasks");
+
+    m_pendingTasksMutex.lock();
+    Deque<TracedTask>& inputTasks = m_pendingInputTasks.swapBuffers();
+    Deque<TracedTask>& compositorTasks = m_pendingCompositorTasks.swapBuffers();
+    m_pendingTasksMutex.unlock();
+
+    for (;;) {
+        if (!inputTasks.isEmpty()) {
+            inputTasks.takeFirst().run();
+            atomicDecrement(&m_highPriotityTaskCount);

[Sami, 2014/08/12 13:02:59]

I think we can replace these atomics with just one atomicSubtract outside the
loop because the task lists won't get modified during execution anymore.

I think we could also rewrite this to be a little simpler:

  while (!inputTasks.isEmpty())
      inputTasks.takeFirst.run();

  while (!compositorTasks.isEmpty())
      compositorTasks.takeFirst.run();

[alexclarke, 2014/08/12 13:59:28]

Done.

+            ASSERT(m_highPriotityTaskCount >= 0);
+            continue;
+        }
+
+        if (compositorTasks.isEmpty())
+            break;
+        compositorTasks.takeFirst().run();
+        atomicDecrement(&m_highPriotityTaskCount);
+        ASSERT(m_highPriotityTaskCount >= 0);
+    }
 }
 
 void Scheduler::sharedTimerAdapter()
@@ -147,7 +228,15 @@ void Scheduler::stopSharedTimer()
 
 bool Scheduler::shouldYieldForHighPriorityWork()
 {
-    return false;
+    return acquireLoad(&m_highPriotityTaskCount) != 0;
+}
+
+void Scheduler::TracedTask::run()
+{
+    TRACE_EVENT2("blink", "TracedTask::run",
+        "src_file", m_location.fileName(),
+        "src_func", m_location.functionName());
+    m_task();
 }
 
 } // namespace blink