gpu: Fix removal of pending queries.

gpu: Fix removal of pending queries.

This moves all queries to a "removed queries" list at removal
time. The memory used by the queries is not freed until the
query has completed.

BUG=253488
TEST=gpu_unittests --gtest_filter=QueryTrackerTest.Remove
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209977

[piman, 2013-07-01 23:18:10]

LGTM, 
Note: be aware of https://codereview.chromium.org/16831004/ which also worked
around the bug, though it kept deleting unfinished queries synchronous. This one
goes further, which is good, It's just that you'll have conflicts.

[apatrick_chromium, 2013-07-01 23:22:35]

lgtm

[reveman, 2013-07-01 23:29:40]

On 2013/07/01 23:18:10, piman wrote:
> LGTM, 
> Note: be aware of https://codereview.chromium.org/16831004/ which also worked
> around the bug, though it kept deleting unfinished queries synchronous. This
one
> goes further, which is good, It's just that you'll have conflicts.

I'm not sure deleting unfinished queries synchronously is enough to completely
solve this problem as async uploads are marked as completed from the upload
thread.

[piman, 2013-07-01 23:32:56]

On Mon, Jul 1, 2013 at 4:29 PM, <reveman@chromium.org> wrote:

> On 2013/07/01 23:18:10, piman wrote:
>
>> LGTM,
>> Note: be aware of
https://codereview.chromium.**org/16831004/<https://codereview.chromium.org/1...
also worked
>> around the bug, though it kept deleting unfinished queries synchronous.
>> This
>>
> one
>
>> goes further, which is good, It's just that you'll have conflicts.
>>
>
> I'm not sure deleting unfinished queries synchronously is enough to
> completely
> solve this problem as async uploads are marked as completed from the upload
> thread.
>

Deleted queries don't write into their shm.

Antoine


>
>
https://codereview.chromium.**org/18340003/<https://codereview.chromium.org/1...
>

[reveman, 2013-07-02 00:20:59]

On 2013/07/01 23:32:56, piman wrote:
> On Mon, Jul 1, 2013 at 4:29 PM, <mailto:reveman@chromium.org> wrote:
> 
> > On 2013/07/01 23:18:10, piman wrote:
> >
> >> LGTM,
> >> Note: be aware of
>
https://codereview.chromium.**org/16831004/%3Chttps://codereview.chromium.org...
> also worked
> >> around the bug, though it kept deleting unfinished queries synchronous.
> >> This
> >>
> > one
> >
> >> goes further, which is good, It's just that you'll have conflicts.
> >>
> >
> > I'm not sure deleting unfinished queries synchronously is enough to
> > completely
> > solve this problem as async uploads are marked as completed from the upload
> > thread.
> >
> 
> Deleted queries don't write into their shm.

hm, that's certainly true for other query types but I don't see how this is
prevented in the case of async pixel transfer queries where
AsyncPixelTransfersCompletedQuery::MarkAsCompletedThreadSafe() is called from
the upload thread that is unaware of if the query has been deleted.

> 
> Antoine
> 
> 
> >
> >
>
https://codereview.chromium.**org/18340003/%3Chttps://codereview.chromium.org...>
> >

[piman, 2013-07-02 00:44:37]

On Mon, Jul 1, 2013 at 5:20 PM, <reveman@chromium.org> wrote:

> On 2013/07/01 23:32:56, piman wrote:
>
>  On Mon, Jul 1, 2013 at 4:29 PM, <mailto:reveman@chromium.org> wrote:
>>
>
>  > On 2013/07/01 23:18:10, piman wrote:
>> >
>> >> LGTM,
>> >> Note: be aware of
>>
>
> https://codereview.chromium.****org/16831004/%3Chttps://codere**
>
view.chromium.org/16831004/%**3Ewhich<http://codereview.chromium.org/16831004/%3Ewhich>
>
>  also worked
>> >> around the bug, though it kept deleting unfinished queries synchronous.
>> >> This
>> >>
>> > one
>> >
>> >> goes further, which is good, It's just that you'll have conflicts.
>> >>
>> >
>> > I'm not sure deleting unfinished queries synchronously is enough to
>> > completely
>> > solve this problem as async uploads are marked as completed from the
>> upload
>> > thread.
>> >
>>
>
>  Deleted queries don't write into their shm.
>>
>
> hm, that's certainly true for other query types but I don't see how this is
> prevented in the case of async pixel transfer queries where
> AsyncPixelTransfersCompletedQu**ery::**MarkAsCompletedThreadSafe() is
> called from
> the upload thread that is unaware of if the query has been deleted.
>

Mmh, is that true? I think that means this change isn't safe either then,
because nothing ensures that the memory isn't reused after the
QuerySyncManager is destroyed, which would race against the query
completion.

I think we need to get stronger guarantees from the service side, otherwise
this is impossible to reason about.



>
>
>  Antoine
>>
>
>
>  >
>> >
>>
>
> https://codereview.chromium.****org/18340003/%3Chttps://codere**
> view.chromium.org/18340003/ <http://codereview.chromium.org/18340003/>>
>
>> >
>>
>
>
>
https://codereview.chromium.**org/18340003/<https://codereview.chromium.org/1...
>

[reveman, 2013-07-02 01:44:08]

On 2013/07/02 00:44:37, piman wrote:
> On Mon, Jul 1, 2013 at 5:20 PM, <mailto:reveman@chromium.org> wrote:
> 
> > On 2013/07/01 23:32:56, piman wrote:
> >
> >  On Mon, Jul 1, 2013 at 4:29 PM, <mailto:reveman@chromium.org> wrote:
> >>
> >
> >  > On 2013/07/01 23:18:10, piman wrote:
> >> >
> >> >> LGTM,
> >> >> Note: be aware of
> >>
> >
> > https://codereview.chromium.****org/16831004/%253Chttps://codere**
> >
>
view.chromium.org/16831004/%**3Ewhich<http://codereview.chromium.org/16831004/%3Ewhich>
> >
> >  also worked
> >> >> around the bug, though it kept deleting unfinished queries synchronous.
> >> >> This
> >> >>
> >> > one
> >> >
> >> >> goes further, which is good, It's just that you'll have conflicts.
> >> >>
> >> >
> >> > I'm not sure deleting unfinished queries synchronously is enough to
> >> > completely
> >> > solve this problem as async uploads are marked as completed from the
> >> upload
> >> > thread.
> >> >
> >>
> >
> >  Deleted queries don't write into their shm.
> >>
> >
> > hm, that's certainly true for other query types but I don't see how this is
> > prevented in the case of async pixel transfer queries where
> > AsyncPixelTransfersCompletedQu**ery::**MarkAsCompletedThreadSafe() is
> > called from
> > the upload thread that is unaware of if the query has been deleted.
> >
> 
> Mmh, is that true? I think that means this change isn't safe either then,
> because nothing ensures that the memory isn't reused after the
> QuerySyncManager is destroyed, which would race against the query
> completion.

Yes, but the lifetime of the QuerySyncManager is currently the same as the
MappedMemoryManager so reuse of the shared mem after destroying the sync manager
isn't really possible at the moment afaict. That's just luck though and not very
future proof.

> 
> I think we need to get stronger guarantees from the service side, otherwise
> this is impossible to reason about.

I agree and created crbug.com/256491

> 
> 
> 
> >
> >
> >  Antoine
> >>
> >
> >
> >  >
> >> >
> >>
> >
> > https://codereview.chromium.****org/18340003/%253Chttps://codere**
> > view.chromium.org/18340003/ <http://codereview.chromium.org/18340003/>>
> >
> >> >
> >>
> >
> >
> >
>
https://codereview.chromium.**org/18340003/%3Chttps://codereview.chromium.org...>
> >

[commit-bot: I haz the power, 2013-07-02 19:50:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/18340003/1

[commit-bot: I haz the power, 2013-07-02 19:57:16]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-07-02 20:07:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/18340003/1

[commit-bot: I haz the power, 2013-07-03 02:01:24]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-03 02:38:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/18340003/1

[commit-bot: I haz the power, 2013-07-03 04:14:45]

Failed to apply patch for gpu/command_buffer/client/gles2_implementation.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file gpu/command_buffer/client/gles2_implementation.cc
  Hunk #1 FAILED at 3174.
  1 out of 1 hunk FAILED -- saving rejects to file
gpu/command_buffer/client/gles2_implementation.cc.rej

Patch:       gpu/command_buffer/client/gles2_implementation.cc
Index: gpu/command_buffer/client/gles2_implementation.cc
diff --git a/gpu/command_buffer/client/gles2_implementation.cc
b/gpu/command_buffer/client/gles2_implementation.cc
index
fbc92d58939c8c39999c73c2d3a23393820a936c..1bc3f0b39f7fbddfe3ef1c9470ba0dbffd374119
100644
--- a/gpu/command_buffer/client/gles2_implementation.cc
+++ b/gpu/command_buffer/client/gles2_implementation.cc
@@ -3174,36 +3174,10 @@ void GLES2Implementation::DeleteQueriesEXTHelper(
         "glDeleteTextures", "id not created by this context.");
     return;
   }
-  // When you delete a query you can't mark its memory as unused until it's
-  // completed.
-  // Note: If you don't do this you won't mess up the service but you will mess
-  // up yourself.
-
-  // TODO(gman): Consider making this faster by putting pending quereies
-  // on some queue to be removed when they are finished.
-  bool query_pending = false;
-  for (GLsizei ii = 0; ii < n; ++ii) {
-    QueryTracker::Query* query = query_tracker_->GetQuery(queries[ii]);
-    if (query && query->Pending()) {
-      query_pending = true;
-      break;
-    }
-  }
 
-  if (query_pending) {
-    WaitForCmd();
-  }
+  for (GLsizei ii = 0; ii < n; ++ii)
+    query_tracker_->RemoveQuery(queries[ii]);
 
-  for (GLsizei ii = 0; ii < n; ++ii) {
-    QueryTracker::Query* query = query_tracker_->GetQuery(queries[ii]);
-    if (query && query->Pending()) {
-      if (!query->CheckResultsAvailable(helper_)) {
-        // Should only get here on context lost.
-        MustBeContextLost();
-      }
-    }
-    query_tracker_->RemoveQuery(queries[ii], helper_->IsContextLost());
-  }
   helper_->DeleteQueriesEXTImmediate(n, queries);
 }

[commit-bot: I haz the power, 2013-07-03 04:21:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/18340003/29001

[commit-bot: I haz the power, 2013-07-03 08:26:48]

Failed to apply patch for gpu/command_buffer/client/gles2_implementation.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file gpu/command_buffer/client/gles2_implementation.cc
  Hunk #1 FAILED at 3174.
  1 out of 1 hunk FAILED -- saving rejects to file
gpu/command_buffer/client/gles2_implementation.cc.rej

Patch:       gpu/command_buffer/client/gles2_implementation.cc
Index: gpu/command_buffer/client/gles2_implementation.cc
diff --git a/gpu/command_buffer/client/gles2_implementation.cc
b/gpu/command_buffer/client/gles2_implementation.cc
index
57dd4e0cfd13dbd4a32f75a416e05cb07330ba23..d152cfa1e37b0457a23d651052d9bbee522e5f02
100644
--- a/gpu/command_buffer/client/gles2_implementation.cc
+++ b/gpu/command_buffer/client/gles2_implementation.cc
@@ -3174,32 +3174,11 @@ void GLES2Implementation::DeleteQueriesEXTHelper(
         "glDeleteTextures", "id not created by this context.");
     return;
   }
-  // When you delete a query you can't mark its memory as unused until it's
-  // either completed, or deleted in the gpu process.
-  // Note: If you don't do this you won't mess up the service but you will mess
-  // up yourself.
-
-  bool query_pending = false;
-  for (GLsizei ii = 0; ii < n; ++ii) {
-    QueryTracker::Query* query = query_tracker_->GetQuery(queries[ii]);
-    if (query && !query->CheckResultsAvailable(helper_)) {
-      query_pending = true;
-      break;
-    }
-  }
 
   helper_->DeleteQueriesEXTImmediate(n, queries);
 
-  if (query_pending) {
-    // This should make sure that the GPU process have deleted the queries
-    // and given up any claim on the shared memory that goes along with
-    // those queries so that we can safely re-use the shared memory.
-    WaitForCmd();
-  }
-
-  for (GLsizei ii = 0; ii < n; ++ii) {
-    query_tracker_->RemoveQuery(queries[ii], helper_->IsContextLost());
-  }
+  for (GLsizei ii = 0; ii < n; ++ii)
+    query_tracker_->RemoveQuery(queries[ii]);
 }
 
 // TODO(gman): Remove this. Queries are not shared resources.

[commit-bot: I haz the power, 2013-07-03 14:24:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/18340003/48001

[commit-bot: I haz the power, 2013-07-03 14:24:42]

Change committed as 209977