cc: Have 1-copy rasterizer issue copy commands at the rate of raster.

cc/resources/one_copy_raster_worker_pool.cc

Index: cc/resources/one_copy_raster_worker_pool.cc
diff --git a/cc/resources/one_copy_raster_worker_pool.cc b/cc/resources/one_copy_raster_worker_pool.cc
index 66b6ea0e22ddde0fe3e51b76481367e234fc27ff..3411b8fc2d2a14c4b74bfd8ffc70a91446287c4f 100644
--- a/cc/resources/one_copy_raster_worker_pool.cc
+++ b/cc/resources/one_copy_raster_worker_pool.cc
@@ -5,6 +5,7 @@
 #include "cc/resources/one_copy_raster_worker_pool.h"
 
 #include <algorithm>
+#include <limits>
 
 #include "base/debug/trace_event.h"
 #include "base/debug/trace_event_argument.h"
@@ -21,23 +22,28 @@ namespace {
 
 class RasterBufferImpl : public RasterBuffer {
  public:
-  RasterBufferImpl(ResourceProvider* resource_provider,
+  RasterBufferImpl(OneCopyRasterWorkerPool* worker_pool,
+                   ResourceProvider* resource_provider,
                    ResourcePool* resource_pool,
                    const Resource* resource)
-      : resource_provider_(resource_provider),
+      : worker_pool_(worker_pool),
+        resource_provider_(resource_provider),
         resource_pool_(resource_pool),
         resource_(resource),
         raster_resource_(resource_pool->AcquireResource(resource->size())),
         lock_(new ResourceProvider::ScopedWriteLockGpuMemoryBuffer(
             resource_provider_,
-            raster_resource_->id())) {}
+            raster_resource_->id())),
+        sequence_(0) {}
 
   ~RasterBufferImpl() override {
-    // First unlock raster resource.
+    // Release write lock in case a copy was never scheduled.
     lock_.reset();
 
-    // Copy contents of raster resource to |resource_|.
-    resource_provider_->CopyResource(raster_resource_->id(), resource_->id());
+    // Make sure any scheduled copy operations are issued before we release the
+    // raster resource.
+    if (sequence_)
+      worker_pool_->IssueCopy(sequence_);
 
     // Return raster resource to pool so it can be used by another RasterBuffer
     // instance.
@@ -54,28 +60,46 @@ class RasterBufferImpl : public RasterBuffer {
       return;
 
     RasterWorkerPool::PlaybackToMemory(gpu_memory_buffer->Map(),
-                                       resource_->format(),
-                                       resource_->size(),
+                                       raster_resource_->format(),
+                                       raster_resource_->size(),
                                        gpu_memory_buffer->GetStride(),
                                        picture_pile,
                                        rect,
                                        scale,
                                        stats);
     gpu_memory_buffer->Unmap();
+
+    sequence_ = worker_pool_->ScheduleCopyOnWorkerThread(
+        lock_.Pass(), raster_resource_.get(), resource_);
   }
 
  private:
+  OneCopyRasterWorkerPool* worker_pool_;
   ResourceProvider* resource_provider_;
   ResourcePool* resource_pool_;
   const Resource* resource_;
   scoped_ptr<ScopedResource> raster_resource_;
   scoped_ptr<ResourceProvider::ScopedWriteLockGpuMemoryBuffer> lock_;
+  CopySequenceNumber sequence_;
 
   DISALLOW_COPY_AND_ASSIGN(RasterBufferImpl);
 };
 
+// Number of resources to copy at a time.
+const int kBatchSize = 4;
+
 }  // namespace
 
+OneCopyRasterWorkerPool::CopyOperation::CopyOperation(
+    scoped_ptr<ResourceProvider::ScopedWriteLockGpuMemoryBuffer> write_lock,
+    ResourceProvider::ResourceId src,
+    ResourceProvider::ResourceId dst)
+    : write_lock(write_lock.Pass()), src(src), dst(dst) {
+}
+
+OneCopyRasterWorkerPool::CopyOperation::~CopyOperation() {
+}
+
 // static
 scoped_ptr<RasterWorkerPool> OneCopyRasterWorkerPool::Create(
     base::SequencedTaskRunner* task_runner,
@@ -103,6 +127,10 @@ OneCopyRasterWorkerPool::OneCopyRasterWorkerPool(
       context_provider_(context_provider),
       resource_provider_(resource_provider),
       resource_pool_(resource_pool),
+      last_issued_copy_operation_(0),
+      last_flushed_copy_operation_(0),
+      next_copy_operation_sequence_(1),
+      weak_ptr_factory_(this),
       raster_finished_weak_ptr_factory_(this) {
   DCHECK(context_provider_);
 }
@@ -201,6 +229,7 @@ void OneCopyRasterWorkerPool::CheckForCompletedTasks() {
 
   task_graph_runner_->CollectCompletedTasks(namespace_token_,
                                             &completed_tasks_);
+
   for (Task::Vector::const_iterator it = completed_tasks_.begin();
        it != completed_tasks_.end();
        ++it) {
@@ -213,15 +242,13 @@ void OneCopyRasterWorkerPool::CheckForCompletedTasks() {
     task->RunReplyOnOriginThread();
   }
   completed_tasks_.clear();
-
-  context_provider_->ContextGL()->ShallowFlushCHROMIUM();
 }
 
 scoped_ptr<RasterBuffer> OneCopyRasterWorkerPool::AcquireBufferForRaster(
     const Resource* resource) {
   DCHECK_EQ(resource->format(), resource_pool_->resource_format());
   return make_scoped_ptr<RasterBuffer>(
-      new RasterBufferImpl(resource_provider_, resource_pool_, resource));
+      new RasterBufferImpl(this, resource_provider_, resource_pool_, resource));
 }
 
 void OneCopyRasterWorkerPool::ReleaseBufferForRaster(
@@ -229,6 +256,48 @@ void OneCopyRasterWorkerPool::ReleaseBufferForRaster(
   // Nothing to do here. RasterBufferImpl destructor cleans up after itself.
 }
 
+CopySequenceNumber OneCopyRasterWorkerPool::ScheduleCopyOnWorkerThread(
+    scoped_ptr<ResourceProvider::ScopedWriteLockGpuMemoryBuffer> write_lock,
+    const Resource* src,
+    const Resource* dst) {
+  base::AutoLock lock(lock_);
+
+  CopySequenceNumber sequence = next_copy_operation_sequence_++;
+
+  pending_copy_operations_.push_back(make_scoped_ptr(
+      new CopyOperation(write_lock.Pass(), src->id(), dst->id())));
+
+  // Post task that will flush a batch of copy operations whenever the number
+  // of pending operations is a multiple of the batch size.
+  if ((pending_copy_operations_.size() % kBatchSize) == 0) {

[vmpstr, 2014/10/22 23:25:16]

We only flush if the pending size grows to be 4? When would we flush if we do 1
schedule, 1 copy, 1 schedule, 1 copy, etc? Or do we not need to flush in that
case?

(I guess what I'm asking is should this be (sequence % kBatchSize) == 0)?)

[reveman, 2014/10/23 15:38:55]

Good idea, I changed it to what you suggested here in latest patch. 

FYI, I don't think it makes much of a difference in practice as it's unlikely
that we'll re-schedule tasks this quickly without any drawing and a flush for
that reason.

+    task_runner_->PostTask(FROM_HERE,
+                           base::Bind(&OneCopyRasterWorkerPool::FlushCopy,
+                                      weak_ptr_factory_.GetWeakPtr(),
+                                      sequence));
+  }
+
+  return sequence;
+}
+
+void OneCopyRasterWorkerPool::IssueCopy(CopySequenceNumber sequence) {

[vmpstr, 2014/10/22 23:25:16]

I think these functions warrant a bit more comments. I'm sitting with a pen and
paper trying to figure out if there's some timing bug lurking here :) I think
it's all good, but it would help if there was a brief explanation of why.

In particular, something like 
"pending copies will always correspond to a monotonically increasing sequence
numbers; issuing a copy of a sequence will ensure that copies are issued for all
sequences that precede it"... Maybe there's more, but I'm pretty convinced that
this works fine (modulo my other comment about kBatchSize). 

[reveman, 2014/10/23 15:38:55]

I changed the naming of these functions in latest patch so it would be more
clear how this works. Let me know if you think some additional comments are
still needed.

+  if (last_issued_copy_operation_ >= sequence)
+    return;
+
+  IssueCopyOperations(sequence - last_issued_copy_operation_);
+  last_issued_copy_operation_ = sequence;
+}
+
+void OneCopyRasterWorkerPool::FlushCopy(CopySequenceNumber sequence) {
+  if (last_flushed_copy_operation_ >= sequence)
+    return;
+
+  IssueCopy(sequence);
+
+  // Flush all issued copy operations.
+  context_provider_->ContextGL()->ShallowFlushCHROMIUM();
+  last_flushed_copy_operation_ = last_issued_copy_operation_;
+}
+
 void OneCopyRasterWorkerPool::OnRasterFinished(TaskSet task_set) {
   TRACE_EVENT1(
       "cc", "OneCopyRasterWorkerPool::OnRasterFinished", "task_set", task_set);
@@ -244,6 +313,32 @@ void OneCopyRasterWorkerPool::OnRasterFinished(TaskSet task_set) {
   client_->DidFinishRunningTasks(task_set);
 }
 
+void OneCopyRasterWorkerPool::IssueCopyOperations(int64 count) {

[vmpstr, 2014/10/22 23:25:16]

I'm not sure if there's a better name for this? Maybe like
IssueGivenCountOfCopies.. that's maybe a bit awkward. If you can come up with
something else, I would prefer a name that's less similar to IssueCopy that also
takes an int64

[reveman, 2014/10/23 15:38:55]

Kept this as is in latest patch after changing the name of some other functions.
Let me know if you still think this needs a better name.

+  TRACE_EVENT1(
+      "cc", "OneCopyRasterWorkerPool::IssueCopyOperations", "count", count);
+
+  CopyOperation::Deque copy_operations;
+
+  {
+    base::AutoLock lock(lock_);
+
+    for (int64 i = 0; i < count; ++i) {
+      DCHECK(!pending_copy_operations_.empty());
+      copy_operations.push_back(pending_copy_operations_.take_front());
+    }
+  }
+
+  while (!copy_operations.empty()) {
+    scoped_ptr<CopyOperation> copy_operation = copy_operations.take_front();
+
+    // Remove the write lock.
+    copy_operation->write_lock.reset();
+
+    // Copy contents of source resource to destination resource.
+    resource_provider_->CopyResource(copy_operation->src, copy_operation->dst);
+  }
+}
+
 scoped_refptr<base::debug::ConvertableToTraceFormat>
 OneCopyRasterWorkerPool::StateAsValue() const {
   scoped_refptr<base::debug::TracedValue> state =