cc: Avoid issuing COMMANDS_COMPLETED queries unless necessary.

cc/output/gl_renderer.cc

Index: cc/output/gl_renderer.cc
diff --git a/cc/output/gl_renderer.cc b/cc/output/gl_renderer.cc
index 7123edf5b850cf2711116023d392c0be92a6fe2f..95c33da1881ed266d9047ae0ca5a4416445dd7de 100644
--- a/cc/output/gl_renderer.cc
+++ b/cc/output/gl_renderer.cc
@@ -63,9 +63,10 @@ namespace {
 class FallbackFence : public ResourceProvider::Fence {
  public:
   explicit FallbackFence(gpu::gles2::GLES2Interface* gl)
-      : gl_(gl), has_passed_(false) {}
+      : gl_(gl), has_passed_(true) {}
 
   // Overridden from ResourceProvider::Fence:
+  virtual void Set() OVERRIDE { has_passed_ = false; }
   virtual bool HasPassed() OVERRIDE {
     if (!has_passed_) {
       has_passed_ = true;
@@ -235,7 +236,7 @@ struct GLRenderer::PendingAsyncReadPixels {
 class GLRenderer::SyncQuery {
  public:
   explicit SyncQuery(gpu::gles2::GLES2Interface* gl)
-      : gl_(gl), query_id_(0u), weak_ptr_factory_(this) {
+      : gl_(gl), query_id_(0u), is_pending_(false), weak_ptr_factory_(this) {
     gl_->GenQueriesEXT(1, &query_id_);
   }
   virtual ~SyncQuery() { gl_->DeleteQueriesEXT(1, &query_id_); }
@@ -244,21 +245,36 @@ class GLRenderer::SyncQuery {
     DCHECK(!weak_ptr_factory_.HasWeakPtrs() || !IsPending());
     // Invalidate weak pointer held by old fence.
     weak_ptr_factory_.InvalidateWeakPtrs();
-    gl_->BeginQueryEXT(GL_COMMANDS_COMPLETED_CHROMIUM, query_id_);
+    is_pending_ = false;
     return make_scoped_refptr<ResourceProvider::Fence>(
         new Fence(weak_ptr_factory_.GetWeakPtr()));
   }
 
-  void End() { gl_->EndQueryEXT(GL_COMMANDS_COMPLETED_CHROMIUM); }
+  void Set() { is_pending_ = true; }
+
+  void End() {
+    if (!is_pending_)
+      return;
+
+    gl_->BeginQueryEXT(GL_COMMANDS_COMPLETED_CHROMIUM, query_id_);

[piman, 2014/07/18 22:19:42]

nit: it's a bit of an abuse. Maybe add a comment saying that BeginQueryEXT on
GL_COMMANDS_COMPLETED_CHROMIUM is effectively a noop relative to GL, so it
doesn't matter where it happens.

It's conceivable that in the future, for out-of-order GPUs (e.g. tilers), a more
optimal fence (with another extension) would need to bracket draw calls as to
not force ordering. Then we'd want to revisit this.

[reveman, 2014/07/18 23:51:06]

Good point. I've updated the patch so we're still issuing BeginQueryEXT before
any drawing commands that depend on the query (seems like a good idea to keep
that behavior if possible) and I've added some comments to make this more clear.

+    gl_->EndQueryEXT(GL_COMMANDS_COMPLETED_CHROMIUM);
+  }
 
   bool IsPending() {
-    unsigned available = 1;
+    if (!is_pending_)
+      return false;
+
+    unsigned result_available = 1;
     gl_->GetQueryObjectuivEXT(
-        query_id_, GL_QUERY_RESULT_AVAILABLE_EXT, &available);
-    return !available;
+        query_id_, GL_QUERY_RESULT_AVAILABLE_EXT, &result_available);
+    is_pending_ = !result_available;
+    return is_pending_;
   }
 
   void Wait() {
+    if (!is_pending_)
+      return;
+
     unsigned result = 0;
     gl_->GetQueryObjectuivEXT(query_id_, GL_QUERY_RESULT_EXT, &result);
   }
@@ -270,6 +286,10 @@ class GLRenderer::SyncQuery {
         : query_(query) {}
 
     // Overridden from ResourceProvider::Fence:
+    virtual void Set() OVERRIDE {
+      DCHECK(query_);
+      query_->Set();
+    }
     virtual bool HasPassed() OVERRIDE {
       return !query_ || !query_->IsPending();
     }
@@ -284,6 +304,7 @@ class GLRenderer::SyncQuery {
 
   gpu::gles2::GLES2Interface* gl_;
   unsigned query_id_;
+  bool is_pending_;
   base::WeakPtrFactory<SyncQuery> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(SyncQuery);