cc: [WIP] Use worker context and OrderingBarrierCHROMIUM for one-copy.

cc/raster/one_copy_tile_task_worker_pool.cc

Index: cc/raster/one_copy_tile_task_worker_pool.cc
diff --git a/cc/raster/one_copy_tile_task_worker_pool.cc b/cc/raster/one_copy_tile_task_worker_pool.cc
index dd8937c483d2cb6b778c6dd03e6b674cbb0b64ec..71cc4c119d995442bb2b1a82fcc65b0dd32fa04e 100644
--- a/cc/raster/one_copy_tile_task_worker_pool.cc
+++ b/cc/raster/one_copy_tile_task_worker_pool.cc
@@ -33,8 +33,7 @@ class RasterBufferImpl : public RasterBuffer {
         resource_provider_(resource_provider),
         resource_pool_(resource_pool),
         output_resource_(output_resource),
-        raster_content_id_(0),
-        sequence_(0) {
+        raster_content_id_(0) {
     if (worker_pool->have_persistent_gpu_memory_buffers() &&
         previous_content_id) {
       raster_resource_ =
@@ -58,11 +57,6 @@ class RasterBufferImpl : public RasterBuffer {
     // Release write lock in case a copy was never scheduled.
     lock_.reset();
 
-    // Make sure any scheduled copy operations are issued before we release the
-    // raster resource.
-    if (sequence_)
-      worker_pool_->AdvanceLastIssuedCopyTo(sequence_);
-
     // Return resources to pool so they can be used by another RasterBuffer
     // instance.
     resource_pool_->ReleaseResource(raster_resource_.Pass(),
@@ -78,7 +72,7 @@ class RasterBufferImpl : public RasterBuffer {
     // If there's a raster_content_id_, we are reusing a resource with that
     // content id.
     bool reusing_raster_resource = raster_content_id_ != 0;
-    sequence_ = worker_pool_->PlaybackAndScheduleCopyOnWorkerThread(
+    worker_pool_->PlaybackAndCopyOnWorkerThread(
         reusing_raster_resource, lock_.Pass(), raster_resource_.get(),
         output_resource_, raster_source, raster_full_rect, raster_dirty_rect,
         scale);
@@ -94,21 +88,10 @@ class RasterBufferImpl : public RasterBuffer {
   uint64_t raster_content_id_;
   scoped_ptr<ScopedResource> raster_resource_;
   scoped_ptr<ResourceProvider::ScopedWriteLockGpuMemoryBuffer> lock_;
-  CopySequenceNumber sequence_;
 
   DISALLOW_COPY_AND_ASSIGN(RasterBufferImpl);
 };
 
-// Number of in-flight copy operations to allow.
-const int kMaxCopyOperations = 32;

[reveman, 2015/06/09 13:30:55]

we still need this

[sohanjg, 2015/06/09 15:59:23]

i cant find it being used in this file, now that we have removed the sync code.
am i missing something ?

[reveman, 2015/06/09 18:00:30]

We still want to limit the number of copy operations that can be in flight.

[sohanjg, 2015/06/11 13:32:25]

Acknowledged.

-
-// Delay been checking for copy operations to complete.
-const int kCheckForCompletedCopyOperationsTickRateMs = 1;
-
-// Number of failed attempts to allow before we perform a check that will
-// wait for copy operations to complete if needed.
-const int kFailedAttemptsBeforeWaitIfNeeded = 256;
-
 }  // namespace
 
 OneCopyTileTaskWorkerPool::CopyOperation::CopyOperation(
@@ -153,14 +136,7 @@ OneCopyTileTaskWorkerPool::OneCopyTileTaskWorkerPool(
       resource_pool_(resource_pool),
       max_bytes_per_copy_operation_(max_bytes_per_copy_operation),
       have_persistent_gpu_memory_buffers_(have_persistent_gpu_memory_buffers),
-      last_issued_copy_operation_(0),
-      last_flushed_copy_operation_(0),
       lock_(),
-      copy_operation_count_cv_(&lock_),
-      bytes_scheduled_since_last_flush_(0),
-      issued_copy_operation_count_(0),
-      next_copy_operation_sequence_(1),
-      check_for_completed_copy_operations_pending_(false),
       shutdown_(false),
       weak_ptr_factory_(this),
       task_set_finished_weak_ptr_factory_(this) {
@@ -186,7 +162,6 @@ void OneCopyTileTaskWorkerPool::Shutdown() {
     base::AutoLock lock(lock_);
 
     shutdown_ = true;
-    copy_operation_count_cv_.Signal();
   }
 
   TaskGraph empty;
@@ -309,8 +284,7 @@ void OneCopyTileTaskWorkerPool::ReleaseBufferForRaster(
   // Nothing to do here. RasterBufferImpl destructor cleans up after itself.
 }
 
-CopySequenceNumber
-OneCopyTileTaskWorkerPool::PlaybackAndScheduleCopyOnWorkerThread(
+void OneCopyTileTaskWorkerPool::PlaybackAndCopyOnWorkerThread(
     bool reusing_raster_resource,
     scoped_ptr<ResourceProvider::ScopedWriteLockGpuMemoryBuffer>
         raster_resource_write_lock,
@@ -344,9 +318,6 @@ OneCopyTileTaskWorkerPool::PlaybackAndScheduleCopyOnWorkerThread(
     gpu_memory_buffer->Unmap();
   }
 
-  base::AutoLock lock(lock_);
-
-  CopySequenceNumber sequence = 0;
   int bytes_per_row = (BitsPerPixel(raster_resource->format()) *
                        raster_resource->size().width()) /
                       8;
@@ -356,87 +327,21 @@ OneCopyTileTaskWorkerPool::PlaybackAndScheduleCopyOnWorkerThread(
   chunk_size_in_rows = MathUtil::RoundUp(chunk_size_in_rows, 4);
   int y = 0;
   int height = raster_resource->size().height();
+  ContextProvider* context_provider =
+      resource_provider_->output_surface()->worker_context_provider();
   while (y < height) {
-    int failed_attempts = 0;
-    while ((pending_copy_operations_.size() + issued_copy_operation_count_) >=
-           kMaxCopyOperations) {
-      // Ignore limit when shutdown is set.
-      if (shutdown_)
-        break;
-
-      ++failed_attempts;
-
-      // Schedule a check that will also wait for operations to complete
-      // after too many failed attempts.
-      bool wait_if_needed = failed_attempts > kFailedAttemptsBeforeWaitIfNeeded;
-
-      // Schedule a check for completed copy operations if too many operations
-      // are currently in-flight.
-      ScheduleCheckForCompletedCopyOperationsWithLockAcquired(wait_if_needed);
-
-      {
-        TRACE_EVENT0("cc", "WaitingForCopyOperationsToComplete");
-
-        // Wait for in-flight copy operations to drop below limit.
-        copy_operation_count_cv_.Wait();
-      }
-    }
-
-    // There may be more work available, so wake up another worker thread.
-    copy_operation_count_cv_.Signal();
-
+    base::AutoLock context_lock(*context_provider->GetLock());
+    context_provider->DetachFromThread();
     // Copy at most |chunk_size_in_rows|.
     int rows_to_copy = std::min(chunk_size_in_rows, height - y);
     DCHECK_GT(rows_to_copy, 0);
-
-    // |raster_resource_write_lock| is passed to the first copy operation as it
-    // needs to be released before we can issue a copy.
-    pending_copy_operations_.push_back(make_scoped_ptr(new CopyOperation(
+    IssueCopyOperations(new CopyOperation(

[reveman, 2015/06/09 13:30:55]

This is leaking a CopyOperations instance. IssueCopyOperations function and
CopyOperations struct are both unnecessary when performing the copy on a worker
thread. 

[sohanjg, 2015/06/09 15:59:23]

Done.

         raster_resource_write_lock.Pass(), raster_resource, output_resource,
-        gfx::Rect(0, y, raster_resource->size().width(), rows_to_copy))));
+        gfx::Rect(0, y, raster_resource->size().width(), rows_to_copy)));
     y += rows_to_copy;
-
-    // Acquire a sequence number for this copy operation.
-    sequence = next_copy_operation_sequence_++;
-
-    // Increment |bytes_scheduled_since_last_flush_| by the amount of memory
-    // used for this copy operation.
-    bytes_scheduled_since_last_flush_ += rows_to_copy * bytes_per_row;
-
-    // Post task that will advance last flushed copy operation to |sequence|
-    // when |bytes_scheduled_since_last_flush_| has reached
-    // |max_bytes_per_copy_operation_|.
-    if (bytes_scheduled_since_last_flush_ >= max_bytes_per_copy_operation_) {
-      task_runner_->PostTask(
-          FROM_HERE,
-          base::Bind(&OneCopyTileTaskWorkerPool::AdvanceLastFlushedCopyTo,
-                     weak_ptr_factory_.GetWeakPtr(), sequence));
-      bytes_scheduled_since_last_flush_ = 0;
-    }
+    context_provider->ContextGL()->OrderingBarrierCHROMIUM();

[reveman, 2015/06/09 13:30:55]

please add comment explaining this ordering barrier

[sohanjg, 2015/06/09 15:59:23]

Done.

+    context_provider->DetachFromThread();
   }
-
-  return sequence;
-}
-
-void OneCopyTileTaskWorkerPool::AdvanceLastIssuedCopyTo(
-    CopySequenceNumber sequence) {
-  if (last_issued_copy_operation_ >= sequence)
-    return;
-
-  IssueCopyOperations(sequence - last_issued_copy_operation_);
-  last_issued_copy_operation_ = sequence;
-}
-
-void OneCopyTileTaskWorkerPool::AdvanceLastFlushedCopyTo(
-    CopySequenceNumber sequence) {
-  if (last_flushed_copy_operation_ >= sequence)
-    return;
-
-  AdvanceLastIssuedCopyTo(sequence);
-
-  // Flush all issued copy operations.
-  context_provider_->ContextGL()->ShallowFlushCHROMIUM();
-  last_flushed_copy_operation_ = last_issued_copy_operation_;
 }
 
 void OneCopyTileTaskWorkerPool::OnTaskSetFinished(TaskSet task_set) {
@@ -454,90 +359,15 @@ void OneCopyTileTaskWorkerPool::OnTaskSetFinished(TaskSet task_set) {
   client_->DidFinishRunningTileTasks(task_set);
 }
 
-void OneCopyTileTaskWorkerPool::IssueCopyOperations(int64 count) {
-  TRACE_EVENT1("cc", "OneCopyTileTaskWorkerPool::IssueCopyOperations", "count",
-               count);
+void OneCopyTileTaskWorkerPool::IssueCopyOperations(
+    CopyOperation* copy_operation) {
+  TRACE_EVENT0("cc", "OneCopyTileTaskWorkerPool::IssueCopyOperations");
 
-  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());
-    }
-
-    // Increment |issued_copy_operation_count_| to reflect the transition of
-    // copy operations from "pending" to "issued" state.
-    issued_copy_operation_count_ += copy_operations.size();
-  }
-
-  while (!copy_operations.empty()) {
-    scoped_ptr<CopyOperation> copy_operation = copy_operations.take_front();
-
-    // Remove the write lock.
-    copy_operation->src_write_lock.reset();
-
-    // Copy contents of source resource to destination resource.
-    resource_provider_->CopyResource(copy_operation->src->id(),
-                                     copy_operation->dst->id(),
-                                     copy_operation->rect);
-  }
-}
-
-void OneCopyTileTaskWorkerPool::
-    ScheduleCheckForCompletedCopyOperationsWithLockAcquired(
-        bool wait_if_needed) {
-  lock_.AssertAcquired();
-
-  if (check_for_completed_copy_operations_pending_)
-    return;
-
-  base::TimeTicks now = base::TimeTicks::Now();
-
-  // Schedule a check for completed copy operations as soon as possible but
-  // don't allow two consecutive checks to be scheduled to run less than the
-  // tick rate apart.
-  base::TimeTicks next_check_for_completed_copy_operations_time =
-      std::max(last_check_for_completed_copy_operations_time_ +
-                   base::TimeDelta::FromMilliseconds(
-                       kCheckForCompletedCopyOperationsTickRateMs),
-               now);
-
-  task_runner_->PostDelayedTask(
-      FROM_HERE,
-      base::Bind(&OneCopyTileTaskWorkerPool::CheckForCompletedCopyOperations,
-                 weak_ptr_factory_.GetWeakPtr(), wait_if_needed),
-      next_check_for_completed_copy_operations_time - now);
-
-  last_check_for_completed_copy_operations_time_ =
-      next_check_for_completed_copy_operations_time;
-  check_for_completed_copy_operations_pending_ = true;
-}
-
-void OneCopyTileTaskWorkerPool::CheckForCompletedCopyOperations(
-    bool wait_if_needed) {
-  TRACE_EVENT1("cc",
-               "OneCopyTileTaskWorkerPool::CheckForCompletedCopyOperations",
-               "wait_if_needed", wait_if_needed);
-
-  resource_pool_->CheckBusyResources(wait_if_needed);
-
-  {
-    base::AutoLock lock(lock_);
-
-    DCHECK(check_for_completed_copy_operations_pending_);
-    check_for_completed_copy_operations_pending_ = false;
-
-    // The number of busy resources in the pool reflects the number of issued
-    // copy operations that have not yet completed.
-    issued_copy_operation_count_ = resource_pool_->busy_resource_count();
-
-    // There may be work blocked on too many in-flight copy operations, so wake
-    // up a worker thread.
-    copy_operation_count_cv_.Signal();
-  }
+  copy_operation->src_write_lock.reset();
+  // Copy contents of source resource to destination resource.
+  resource_provider_->CopyResource(copy_operation->src->id(),

[reveman, 2015/06/09 13:30:55]

We can't use ResourceProvider on this thread.

[sohanjg, 2015/06/09 15:59:23]

Done.

+                                   copy_operation->dst->id(),
+                                   copy_operation->rect);
 }
 
 scoped_refptr<base::trace_event::ConvertableToTraceFormat>