Implement idle task scheduling.

Source/platform/scheduler/TracedTask.cpp

Index: Source/platform/scheduler/TracedTask.cpp
diff --git a/Source/platform/scheduler/TracedTask.cpp b/Source/platform/scheduler/TracedTask.cpp
index d4e1fa69f78c4535379dfbff2f49e00138acad4f..3cbed4ca998a559b1405170f040dddac126dc782 100644
--- a/Source/platform/scheduler/TracedTask.cpp
+++ b/Source/platform/scheduler/TracedTask.cpp
@@ -4,37 +4,68 @@
 
 #include "config.h"
 #include "platform/scheduler/TracedTask.h"
+#include "platform/scheduler/Scheduler.h"
 
 namespace blink {
 
 volatile int TracedTask::s_nextFlowTraceID = 0;
 
-void TracedTask::run() const
+TracedTask::TracedTask(const TraceLocation& location, const char* traceName)
+    : m_location(location)
+    , m_traceName(traceName)
+{
+    bool tracingEnabled;
+    TRACE_EVENT_CATEGORY_GROUP_ENABLED("blink", &tracingEnabled);
+
+    if (tracingEnabled) {
+        // atomicIncrement is slow so we only do it if tracing is enabled
+        m_flowTraceID = static_cast<uint64_t>(atomicIncrement(&s_nextFlowTraceID));
+
+        TRACE_EVENT_FLOW_BEGIN2("blink", m_traceName, MANGLE(m_flowTraceID),
+        "src_file", m_location.fileName(),
+        "src_func", m_location.functionName());
+    }
+}
+
+TracedTask::~TracedTask() { }
+
+void TracedTask::traceRun() const
 {
     TRACE_EVENT_FLOW_END0("blink", m_traceName, MANGLE(m_flowTraceID));
 
     TRACE_EVENT2("blink", m_traceName,
         "src_file", m_location.fileName(),
         "src_func", m_location.functionName());

[picksi1, 2014/09/25 08:53:31]

This function is confusingly named, how about updateTraceEvents() or something.
Also AFAIK the TRACE_EVENT2 macro needs to be in the same scope as the execution
of the task to correctly measure its duration... Sami will be able to confirm or
deny this.

[rmcilroy, 2014/09/29 17:42:58]

Changed to endFlowTraceEvent() (and only deals with the flow event now).
Yes you are right - oops. Moved to the appropriate scope.

+}
+
+TracedStandardTask::TracedStandardTask(const Task& task, const TraceLocation& location, const char* traceName)
+    : TracedTask(location, traceName)
+    , m_task(task) { }
 
+TracedStandardTask::~TracedStandardTask() { }
+
+void TracedStandardTask::run() const
+{
+    traceRun();
     m_task();
 }
 
-TracedTask::TracedTask(const Task& task, const TraceLocation& location, const char* traceName)
-    : m_task(task)
-    , m_location(location)
-    , m_traceName(traceName)
+TracedIdleTask::TracedIdleTask(const IdleTask& idleTask, const TraceLocation& location, const char* traceName)
+    : TracedTask(location, traceName)
+    , m_idleTask(idleTask)
 {
-    bool tracingEnabled;
-    TRACE_EVENT_CATEGORY_GROUP_ENABLED("blink", &tracingEnabled);
+    ASSERT(Scheduler::shared());
+}

[picksi1, 2014/09/25 08:53:31]

Do we need this ASSERT here? Might there be a situation were we create tasks
ahead of time (e.g. dummy tasks or something)? Should the assert be in the run()
function instead?

[rmcilroy, 2014/09/29 17:42:57]

I think these should only be created by the Scheduler, so this assert should
pass I would have thought - not sure though, happy to move if either Sami or you
think that it might fail in certain situations.

 
-    if (tracingEnabled) {
-        // atomicIncrement is slow so we only do it if tracing is enabled
-        m_flowTraceID = static_cast<uint64_t>(atomicIncrement(&s_nextFlowTraceID));
+TracedIdleTask::~TracedIdleTask() { }
 
-        TRACE_EVENT_FLOW_BEGIN2("blink", m_traceName, MANGLE(m_flowTraceID),
-        "src_file", m_location.fileName(),
-        "src_func", m_location.functionName());
+void TracedIdleTask::run() const
+{
+    Scheduler* scheduler = Scheduler::shared();

[Sami, 2014/09/24 14:00:40]

Another note to self: we should keep |scheduler| as a member to avoid the global
lookup.

[rmcilroy, 2014/09/29 17:42:58]

Acknowledged.

+    double availableIdleTime = scheduler->getSecondsUntilFrameDeadline();

[rmcilroy, 2014/09/24 08:49:18]

Simon: I'm not entirely convinced that it is worth making the TracedIdleTask
subclass from TracedTask currently.  There doesn't seem to be much benifit from
both pendingTask queues being queue's of the same type since we need to execute
them differently in the scheduler anyway (see Scheduler::executeIdleTasks()),
and the requirement the TracedIdleTask::run() has no-args means that we need to
call scheduler->getSecondsUntilFrameDeadline() from here as well as
Scheduler::executeIdleTasks(), when otherwise it could be passed as an argument
to this function.  WDYT?

[picksi1, 2014/09/25 08:53:31]

I see what you mean. I wonder if there's the beginning of a pattern here though
- different types of tasks have different conditions that allow their
bulk-execution to even be considered; Idle tasks need sufficient time,
HighPriority tasks need to have the correct policy setting (... etc?).

Also having polymorphic tasks will be really helpful if we ever want to  have a
single point of task execution within the scheduler (which I'd favour as it aids
understandability and debugging).

It's certainly worth more discussion though.

[rmcilroy, 2014/09/29 17:42:57]

The fact that the new patch set passes a deadline rather than an allotted time
makes me more amenable to this approach, since it avoids the need to call the
expensive getSecondsUntilFrameDeadline() function twice
(currentFrameDeadlineSeconds is a much cheaper function since it doesn't call
Platform::monotonicallyIncreasingTime()).

+    if (availableIdleTime > 0) {

[Sami, 2014/09/24 14:00:40]

I think there's a race here where the scheduler can think it ran an idle task
but in this function we actually bailed out => the task got dropped.

[rmcilroy, 2014/09/29 17:42:58]

Your right - in the maybeExecuteIdleTask function returns true if it posted an
IdleTask on the main thread, but that doesn't specify that the task will be
executed (we can't know at that point).  In either case though, the task will
either be executed or reposted to the pending queue, so I think this should be
good enough.

+        traceRun();
+        m_idleTask(availableIdleTime * 1000);

[picksi1, 2014/09/25 08:53:31]

Are we able to choose a time unit, seconds or milliseconds, rather than swapping
between the two? Or are our hands tied by other APIs?

[rmcilroy, 2014/09/29 17:42:57]

Change all APIs to seconds.

     }
 }