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

gpu/command_buffer/service/query_manager.cc

 // Copyright (c) 2012 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 "gpu/command_buffer/service/query_manager.h"
 #include "base/atomicops.h"
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/time.h"
 #include "gpu/command_buffer/common/gles2_cmd_format.h"
@@ skipping 158 matching lines @@
     , public base::SupportsWeakPtr<AsyncPixelTransfersCompletedQuery> {
  public:
   AsyncPixelTransfersCompletedQuery(
       QueryManager* manager, GLenum target, int32 shm_id, uint32 shm_offset);
 
   virtual bool Begin() OVERRIDE;
   virtual bool End(uint32 submit_count) OVERRIDE;
   virtual bool Process() OVERRIDE;
   virtual void Destroy(bool have_context) OVERRIDE;
 
-  void MarkAsCompletedCallback() { MarkAsCompleted(1); }
-
  protected:
   virtual ~AsyncPixelTransfersCompletedQuery();
 };
 
 AsyncPixelTransfersCompletedQuery::AsyncPixelTransfersCompletedQuery(
     QueryManager* manager, GLenum target, int32 shm_id, uint32 shm_offset)
     : Query(manager, target, shm_id, shm_offset) {
 }
 
 bool AsyncPixelTransfersCompletedQuery::Begin() {
   return true;
 }
 
 bool AsyncPixelTransfersCompletedQuery::End(uint32 submit_count) {
-  MarkAsPending(submit_count);
+  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 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.
+  // 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()));
+      mem_params, submit_count);
 
-  // TODO(epenner): The async task occurs outside the normal
-  // flow, via a callback on this thread. Is there anything
-  // missing or wrong with that?
+  return AddToPendingTransferQueue(submit_count);
+}
 
-  // TODO(epenner): Could we possibly trigger the completion on
-  // the upload thread by writing to the query shared memory
-  // directly?
+bool AsyncPixelTransfersCompletedQuery::Process() {
+  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;
 }
 
-bool AsyncPixelTransfersCompletedQuery::Process() {
-  NOTREACHED();
-  return true;
-}
-
 void AsyncPixelTransfersCompletedQuery::Destroy(bool /* have_context */) {
   if (!IsDeleted()) {
     MarkAsDeleted();
   }
 }
 
 AsyncPixelTransfersCompletedQuery::~AsyncPixelTransfersCompletedQuery() {
 }
 
 class GetErrorQuery : public QueryManager::Query {
@@ skipping 198 matching lines @@
   return true;
 }
 
 bool QueryManager::ProcessPendingQueries() {
   while (!pending_queries_.empty()) {
     Query* query = pending_queries_.front().get();
     if (!query->Process()) {
       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) {
   DCHECK(query);
   DCHECK(!query->IsDeleted());
   if (!RemovePendingQuery(query)) {
     return false;
   }
   query->MarkAsPending(submit_count);
   pending_queries_.push_back(query);
   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()) {
     // TODO(gman): Speed this up if this is a common operation. This would only
     // happen if you do being/end begin/end on the same query without waiting
     // for the first one to finish.
     for (QueryQueue::iterator it = pending_queries_.begin();
          it != pending_queries_.end(); ++it) {
       if (it->get() == query) {
         pending_queries_.erase(it);
         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;
     }
   }
   return true;
 }
 
 bool QueryManager::BeginQuery(Query* query) {
   DCHECK(query);
   if (!RemovePendingQuery(query)) {
     return false;
   }
   return query->Begin();
 }
 
 bool QueryManager::EndQuery(Query* query, uint32 submit_count) {
   DCHECK(query);
   if (!RemovePendingQuery(query)) {
     return false;
   }
   return query->End(submit_count);
 }
 
 }  // namespace gles2
 }  // namespace gpu