Re-land: Mark async texture uploads as completed from the upload thread.

gpu/command_buffer/service/query_manager.cc

Index: gpu/command_buffer/service/query_manager.cc
diff --git a/gpu/command_buffer/service/query_manager.cc b/gpu/command_buffer/service/query_manager.cc
index 51055e2034a46fa5012ad2cc9c046903bfa24010..c25545f0f77a1cd6e82359091e3139218c8aca6e 100644
--- a/gpu/command_buffer/service/query_manager.cc
+++ b/gpu/command_buffer/service/query_manager.cc
@@ -176,8 +176,6 @@ class AsyncPixelTransfersCompletedQuery
   virtual bool Process() OVERRIDE;
   virtual void Destroy(bool have_context) OVERRIDE;
 
-  void MarkAsCompletedCallback() { MarkAsCompleted(1); }
-
  protected:
   virtual ~AsyncPixelTransfersCompletedQuery();
 };
@@ -192,28 +190,34 @@ bool AsyncPixelTransfersCompletedQuery::Begin() {
 }
 
 bool AsyncPixelTransfersCompletedQuery::End(uint32 submit_count) {
-  MarkAsPending(submit_count);
-
-  // This will call MarkAsCompleted(1) as a reply to a task on
-  // the async upload thread, such that it occurs after all previous
-  // async transfers have completed.
+  gfx::AsyncMemoryParams mem_params;
+  // Get the real shared memory since it might need to be duped to prevent
+  // use-after-free of the memory.
+  Buffer buffer = manager()->decoder()->GetSharedMemoryBuffer(shm_id());
+  mem_params.shared_memory = buffer.shared_memory;
+  mem_params.shm_size = buffer.size;
+  mem_params.shm_data_offset = shm_offset();
+  mem_params.shm_data_size = sizeof(QuerySync);

[epenner, 2013/02/07 20:22:56]

I was in debate of whether we needed to do this in this instance. The Query
class is actually ref-counted... So if that is legit, we could insure a
gpu-thread reference is kept for the duration of the async call.

However, I'm suspicious of the Query being reference counted in the first place.
Can it really outlive its manager safely? This is part of the reason I was
hesitant to do query completion on the thread. I also used a weak pointer rather
than holding a reference, to be sure.

If we keep the dupe, we should investigate removing it along with the texture
memory dupe. We just need an intermediate class that can be reference counted
within a process.

[reveman, 2013/02/07 21:30:52]

I think the safe way to do this for now is duplicating it and we can later
change it if possible along with the texture memory.

+
+  // This tells AsyncPixelTransferDelegate to mark QuerySync as completed
+  // after all previous async transfers are done.
   manager()->decoder()->GetAsyncPixelTransferDelegate()->AsyncNotifyCompletion(

[epenner, 2013/02/07 20:22:56]

I think to avoid the layering violation, we could do the query setting via a
callback that we create right here and pass along with the memory etc. The
delegate can decide whether it needs to dupe the memory, and when/what-thread to
execute the callback on. The callback will need to be very minimal in what it
does, since it could happen on another thread.

[reveman, 2013/02/07 21:30:52]

yea, sounds like that might be a pretty clean solution. and it will get rid of
that inappropriate include, yay!

-      base::Bind(
-          &AsyncPixelTransfersCompletedQuery::MarkAsCompletedCallback,
-          AsWeakPtr()));
-
-  // TODO(epenner): The async task occurs outside the normal
-  // flow, via a callback on this thread. Is there anything
-  // missing or wrong with that?
+      mem_params, submit_count);
 
-  // TODO(epenner): Could we possibly trigger the completion on
-  // the upload thread by writing to the query shared memory
-  // directly?
-  return true;
+  return AddToPendingTransferQueue(submit_count);
 }
 
 bool AsyncPixelTransfersCompletedQuery::Process() {
-  NOTREACHED();
+  QuerySync* sync = manager()->decoder()->GetSharedMemoryAs<QuerySync*>(
+      shm_id(), shm_offset(), sizeof(*sync));
+  if (!sync)
+    return false;
+
+  // Check if AsyncPixelTransferDelegate has marked QuerySync as completed.
+  if (sync->process_count != submit_count())

[epenner, 2013/02/07 20:22:56]

Is this thread safe? Given it's simplicity maybe it is? I think we should add a
comment at the minimum that we are comparing something that is being changed on
another thread.

[reveman, 2013/02/07 21:30:52]

this is consistent with the code in the client side query tracker. the query
system already guarantees that this is atomic across processes. I think that
adding some additional thread safety guards would just be confusing. I'll add a
comment though.

+    return true;
+
+  UnmarkAsPending();
   return true;
 }
 
@@ -435,16 +439,26 @@ bool QueryManager::ProcessPendingQueries() {
       return false;
     }
     if (query->pending()) {
-      return true;
+      break;
     }
     pending_queries_.pop_front();
   }
+  while (!pending_transfer_queries_.empty()) {
+    Query* query = pending_transfer_queries_.front().get();
+    if (!query->Process()) {
+      return false;
+    }
+    if (query->pending()) {
+      break;
+    }
+    pending_transfer_queries_.pop_front();
+  }
 
   return true;
 }
 
 bool QueryManager::HavePendingQueries() {
-  return !pending_queries_.empty();
+  return !pending_queries_.empty() || !pending_transfer_queries_.empty();
 }
 
 bool QueryManager::AddPendingQuery(Query* query, uint32 submit_count) {
@@ -458,6 +472,17 @@ bool QueryManager::AddPendingQuery(Query* query, uint32 submit_count) {
   return true;
 }
 
+bool QueryManager::AddPendingTransferQuery(Query* query, uint32 submit_count) {
+  DCHECK(query);
+  DCHECK(!query->IsDeleted());
+  if (!RemovePendingQuery(query)) {
+    return false;
+  }
+  query->MarkAsPending(submit_count);
+  pending_transfer_queries_.push_back(query);
+  return true;
+}
+
 bool QueryManager::RemovePendingQuery(Query* query) {
   DCHECK(query);
   if (query->pending()) {
@@ -471,6 +496,13 @@ bool QueryManager::RemovePendingQuery(Query* query) {
         break;
       }
     }
+    for (QueryQueue::iterator it = pending_transfer_queries_.begin();
+         it != pending_transfer_queries_.end(); ++it) {
+      if (it->get() == query) {
+        pending_transfer_queries_.erase(it);
+        break;
+      }
+    }
     if (!query->MarkAsCompleted(0)) {
       return false;
     }