Add synthetic delay points for latency testing

cc/resources/raster_worker_pool.cc

Index: cc/resources/raster_worker_pool.cc
diff --git a/cc/resources/raster_worker_pool.cc b/cc/resources/raster_worker_pool.cc
index 06fd18e62e0ec2bdc9d440ee0ae6eb925c867ef4..d3bf17b0f4abaab8fabe3f50a50a3f4b2f12fcc2 100644
--- a/cc/resources/raster_worker_pool.cc
+++ b/cc/resources/raster_worker_pool.cc
@@ -4,6 +4,7 @@
 
 #include "cc/resources/raster_worker_pool.h"
 
+#include "base/debug/trace_event_synthetic_delay.h"
 #include "base/json/json_writer.h"
 #include "base/metrics/histogram.h"
 #include "base/values.h"
@@ -35,6 +36,20 @@ class IdentityAllocator : public SkBitmap::Allocator {
 // a tile is of solid color.
 const bool kUseColorEstimator = true;
 
+template <const char* delay_name>

[reveman, 2013/12/18 20:55:18]

As this is only used for cc.RasterRequiredForActivation, can we remove the
template specialization and name it
RasterRequiredForActivationSyntheticDelayInitializer? ::Lookup(delay_name) ->
::Lookup("cc.RasterRequiredForActivation")..

[Sami, 2013/12/19 12:49:21]

Good point, done.

+struct SyntheticDelayInitializer {
+  SyntheticDelayInitializer()
+      : delay(base::debug::TraceEventSyntheticDelay::Lookup(delay_name)) {}
+  base::debug::TraceEventSyntheticDelay* delay;
+};
+
+// Synthetic delay for raster tasks that are required for activation. Global to
+// avoid static initializer on critical path.
+const char kRasterRequiredForActivation[] = "cc.RasterRequiredForActivation";
+static base::LazyInstance<
+    SyntheticDelayInitializer<kRasterRequiredForActivation> >
+    g_raster_required_for_activation_delay = LAZY_INSTANCE_INITIALIZER;
+
 class DisableLCDTextFilter : public SkDrawFilter {
  public:
   // SkDrawFilter interface.
@@ -291,14 +306,24 @@ class RasterFinishedWorkerPoolTaskImpl : public internal::WorkerPoolTask {
       Callback;
 
   RasterFinishedWorkerPoolTaskImpl(
-      const Callback& on_raster_finished_callback)
+      const Callback& on_raster_finished_callback,
+      bool has_tasks_required_for_activation)
       : origin_loop_(base::MessageLoopProxy::current().get()),
-        on_raster_finished_callback_(on_raster_finished_callback) {
+        on_raster_finished_callback_(on_raster_finished_callback),
+        has_tasks_required_for_activation_(has_tasks_required_for_activation) {
+    if (has_tasks_required_for_activation_) {
+      g_raster_required_for_activation_delay.Get().delay->BeginParallel(
+          &activation_delay_end_time_);
+    }
   }
 
   // Overridden from internal::WorkerPoolTask:
   virtual void RunOnWorkerThread(unsigned thread_index) OVERRIDE {
     TRACE_EVENT0("cc", "RasterFinishedWorkerPoolTaskImpl::RunOnWorkerThread");
+    if (has_tasks_required_for_activation_) {
+      g_raster_required_for_activation_delay.Get().delay->EndParallel(
+          activation_delay_end_time_);
+    }
     origin_loop_->PostTask(
         FROM_HERE,
         base::Bind(&RasterFinishedWorkerPoolTaskImpl::RunOnOriginThread,
@@ -315,6 +340,8 @@ class RasterFinishedWorkerPoolTaskImpl : public internal::WorkerPoolTask {
 
   scoped_refptr<base::MessageLoopProxy> origin_loop_;
   const Callback on_raster_finished_callback_;
+  const bool has_tasks_required_for_activation_;
+  base::TimeTicks activation_delay_end_time_;

[reveman, 2013/12/18 20:55:18]

Maybe cleaner to add a RasterRequiredForActivationFinishedWorkerPoolTaskImpl
subclass that contains this new logic. Especially if we don't need the
has_tasks_required_for_activation_ conditional. Wdyt?

[Sami, 2013/12/19 12:49:21]

Agreed, that's much nicer.

 
   DISALLOW_COPY_AND_ASSIGN(RasterFinishedWorkerPoolTaskImpl);
 };
@@ -502,15 +529,18 @@ scoped_refptr<internal::WorkerPoolTask>
   return make_scoped_refptr(
       new RasterFinishedWorkerPoolTaskImpl(
           base::Bind(&RasterWorkerPool::OnRasterFinished,
-                     weak_ptr_factory_.GetWeakPtr())));
+                     weak_ptr_factory_.GetWeakPtr()),
+          false));
 }
 
 scoped_refptr<internal::WorkerPoolTask>
-    RasterWorkerPool::CreateRasterRequiredForActivationFinishedTask() {
+    RasterWorkerPool::CreateRasterRequiredForActivationFinishedTask(
+       bool has_tasks_required_for_activation) {

[reveman, 2013/12/18 20:55:18]

Is it necessary to avoid this when has_tasks_required_for_activation is false?
Maybe we shouldn't create the task in the first place in that case..

[Sami, 2013/12/19 12:49:21]

The problem is that having no tasks required for activation happens pretty
often. If we apply the delay in that case, the raster worker pool ends up
spinning in a delay all the time and the browser grinds to a halt.
That would be a reasonable thing to do, but has an unfortunate side effect for
delays: if we've finished all the actual raster tasks and are spinning in the
delay, CheckForCompletedRasterTasks can get triggered on the 6 ms timer and
notice that all raster tasks have completed. This means we'll activate the tree
before the delay completes.

I don't have a good idea for working around this without always waiting for
RasterRequiredForActivationFinishedTask. Potentially we could skip the fast path
if the delay is active, but I wouldn't want to change production code too much
to accomodate delays.

[reveman, 2013/12/19 17:29:28]

OK, I can see why that happens.
I thought some more about this and I think the current approach is good. After
all, we want to simulate that the raster tasks required for activation take
longer to complete, not the finish task. I think if the delay is defined as the
product of the number of tasks required for activation and some per-task delay
then it makes perfect sense.

Current code with a constant delay when number of tasks are more than zero is
fine though. That's just a special case where "delay = min(task_count *
kPerTaskDelay, kPerTaskDelay)"  

Maybe just change "bool has_tasks_required_for_activation" here to "size_t
tasks_required_for_activation_count" so the fact that we use a constant delay is
not exposed outside the RasterWorkerPool implementation.

   return make_scoped_refptr(
       new RasterFinishedWorkerPoolTaskImpl(
           base::Bind(&RasterWorkerPool::OnRasterRequiredForActivationFinished,
-                     weak_ptr_factory_.GetWeakPtr())));
+                     weak_ptr_factory_.GetWeakPtr()),
+          has_tasks_required_for_activation));
 }
 
 void RasterWorkerPool::OnRasterFinished(