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..0cbfe46f031d462774d740293c3800022d14533a 100644
--- a/Source/platform/scheduler/Scheduler.cpp
+++ b/Source/platform/scheduler/Scheduler.cpp
@@ -5,59 +5,89 @@
 #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());
     }
 
     // WebThread::Task implementation.
     virtual void run() OVERRIDE
     {
-        TRACE_EVENT1("blink", "MainThreadIdleTaskAdapter::run", "allottedTime", m_allottedTimeMs);
-        m_idleTask(m_allottedTimeMs);
+        Scheduler* scheduler = Scheduler::shared();

[eseidel, 2014/08/12 16:16:01]

We should add a FIXME that this should be removed once we make shutdown
deterministic.

[alexclarke, 2014/08/13 10:08:58]

Done.

+        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());
+    }
+
+    // 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;
 
@@ -78,50 +108,86 @@ Scheduler* Scheduler::shared()
 }
 
 Scheduler::Scheduler()
-    : m_mainThread(blink::Platform::current()->currentThread())
-    , m_sharedTimerFunction(nullptr)
+    : m_sharedTimerFunction(nullptr)
+    , m_mainThread(blink::Platform::current()->currentThread())
+    , m_highPriorityTaskCount(0)
 {
 }
 
 Scheduler::~Scheduler()
 {
+    while (hasPendingHighPriorityWork()) {
+        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_highPriorityTaskCount);
+    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_highPriorityTaskCount);
+    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();

[eseidel, 2014/08/12 16:16:01]

Because this is a bit fragile (if someone called this method on another thread,
for instance) I mgith explain what is keepign this safe.

I might also add an ASSERT(isMainThread()) to the top of this method.

[alexclarke, 2014/08/13 10:08:58]

Done.

+    Deque<TracedTask>& compositorTasks = m_pendingCompositorTasks.swapBuffers();
+    m_pendingTasksMutex.unlock();
+
+    int highPriorityTasksExecuted = 0;
+    while (!inputTasks.isEmpty()) {
+        inputTasks.takeFirst().run();
+        highPriorityTasksExecuted++;
+    }
+
+    while (!compositorTasks.isEmpty()) {
+        compositorTasks.takeFirst().run();
+        highPriorityTasksExecuted++;
+    }
+
+    int highPriorityTaskCount = atomicSubtract(&m_highPriorityTaskCount, highPriorityTasksExecuted);
+    ASSERT_UNUSED(highPriorityTaskCount, highPriorityTaskCount >= 0);
 }
 
 void Scheduler::sharedTimerAdapter()
@@ -145,9 +211,22 @@ void Scheduler::stopSharedTimer()
     blink::Platform::current()->stopSharedTimer();
 }
 
-bool Scheduler::shouldYieldForHighPriorityWork()
+bool Scheduler::shouldYieldForHighPriorityWork() const
+{
+    return hasPendingHighPriorityWork();
+}
+
+bool Scheduler::hasPendingHighPriorityWork() const
+{
+    return acquireLoad(&m_highPriorityTaskCount) != 0;

[eseidel, 2014/08/12 16:16:01]

We should add a (possibly large) comment here to explain why we chose to use
barrier loads here and teh consquences of not.

We believe we don't actually need to do this, that failing to read this bool
corectly would just result in delaying high priority tasks a little, etc.

[alexclarke, 2014/08/13 10:08:58]

Done.

+}
+
+void Scheduler::TracedTask::run()
 {
-    return false;
+    TRACE_EVENT2("blink", "TracedTask::run",
+        "src_file", m_location.fileName(),
+        "src_func", m_location.functionName());
+    m_task();
 }
 
 } // namespace blink