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

cc/resources/one_copy_raster_worker_pool.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "base/strings/stringprintf.h"
 #include "cc/debug/traced_value.h"
 #include "cc/resources/raster_buffer.h"
 #include "cc/resources/resource_pool.h"
 #include "cc/resources/scoped_resource.h"
 #include "gpu/command_buffer/client/gles2_interface.h"
 #include "ui/gfx/gpu_memory_buffer.h"
 
 namespace cc {
 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.
     resource_pool_->ReleaseResource(raster_resource_.Pass());
   }
 
   // Overridden from RasterBuffer:
   void Playback(const PicturePileImpl* picture_pile,
                 const gfx::Rect& rect,
                 float scale,
                 RenderingStatsInstrumentation* stats) override {
     gfx::GpuMemoryBuffer* gpu_memory_buffer = lock_->gpu_memory_buffer();
     if (!gpu_memory_buffer)
       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,
     TaskGraphRunner* task_graph_runner,
     ContextProvider* context_provider,
     ResourceProvider* resource_provider,
     ResourcePool* resource_pool) {
   return make_scoped_ptr<RasterWorkerPool>(
       new OneCopyRasterWorkerPool(task_runner,
                                   task_graph_runner,
                                   context_provider,
                                   resource_provider,
                                   resource_pool));
 }
 
 OneCopyRasterWorkerPool::OneCopyRasterWorkerPool(
     base::SequencedTaskRunner* task_runner,
     TaskGraphRunner* task_graph_runner,
     ContextProvider* context_provider,
     ResourceProvider* resource_provider,
     ResourcePool* resource_pool)
     : task_runner_(task_runner),
       task_graph_runner_(task_graph_runner),
       namespace_token_(task_graph_runner->GetNamespaceToken()),
       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_);
 }
 
 OneCopyRasterWorkerPool::~OneCopyRasterWorkerPool() {
 }
 
 Rasterizer* OneCopyRasterWorkerPool::AsRasterizer() {
   return this;
 }
@@ skipping 78 matching lines @@
 
   TRACE_EVENT_ASYNC_STEP_INTO1(
       "cc", "ScheduledTasks", this, "rasterizing", "state", StateAsValue());
 }
 
 void OneCopyRasterWorkerPool::CheckForCompletedTasks() {
   TRACE_EVENT0("cc", "OneCopyRasterWorkerPool::CheckForCompletedTasks");
 
   task_graph_runner_->CollectCompletedTasks(namespace_token_,
                                             &completed_tasks_);
+
   for (Task::Vector::const_iterator it = completed_tasks_.begin();
        it != completed_tasks_.end();
        ++it) {
     RasterizerTask* task = static_cast<RasterizerTask*>(it->get());
 
     task->WillComplete();
     task->CompleteOnOriginThread(this);
     task->DidComplete();
 
     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(
     scoped_ptr<RasterBuffer> buffer) {
   // 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);
 
   DCHECK(raster_pending_[task_set]);
   raster_pending_[task_set] = false;
   if (raster_pending_.any()) {
     TRACE_EVENT_ASYNC_STEP_INTO1(
         "cc", "ScheduledTasks", this, "rasterizing", "state", StateAsValue());
   } else {
     TRACE_EVENT_ASYNC_END0("cc", "ScheduledTasks", this);
   }
   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 =
       new base::debug::TracedValue();
 
   state->BeginArray("tasks_pending");
   for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; ++task_set)
     state->AppendBoolean(raster_pending_[task_set]);
   state->EndArray();
   state->BeginDictionary("staging_state");
@@ skipping 10 matching lines @@
                             resource_pool_->total_memory_usage_bytes());
   staging_state->SetInteger("pending_copy_count",
                             resource_pool_->total_resource_count() -
                                 resource_pool_->acquired_resource_count());
   staging_state->SetInteger("bytes_pending_copy",
                             resource_pool_->total_memory_usage_bytes() -
                                 resource_pool_->acquired_memory_usage_bytes());
 }
 
 }  // namespace cc