Reduce the number of flushes while using glQueries.

Reduce the number of flushes while using glQueries.

BUG=none

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

[rptr, 2014-04-08 17:43:10]

Please take a look.

This patch avoids few wasteful flush commands after EndQuery, if we have already
flushed through helper. 
Uses already existing flush_generation counter in cmd_buf_helper instead of
recording flush counts separately in   query_tracker.

[Zhenyao Mo, 2014-04-08 18:55:39]

https://chromiumcodereview.appspot.com/228653004/diff/1/gpu/command_buffer/cl...
File gpu/command_buffer/client/query_tracker.cc (right):

https://chromiumcodereview.appspot.com/228653004/diff/1/gpu/command_buffer/cl...
gpu/command_buffer/client/query_tracker.cc:175: ++last_flush_;
This should be last_flush_ = helper->flush_generation() instead.

[rptr, 2014-04-08 20:58:49]

PTAL.

https://chromiumcodereview.appspot.com/228653004/diff/1/gpu/command_buffer/cl...
File gpu/command_buffer/client/query_tracker.cc (right):

https://chromiumcodereview.appspot.com/228653004/diff/1/gpu/command_buffer/cl...
gpu/command_buffer/client/query_tracker.cc:175: ++last_flush_;
On 2014/04/08 18:55:40, Zhenyao Mo wrote:
> This should be last_flush_ = helper->flush_generation() instead.
Yes I agree. Better to set/get flush_count from a common place.

[Zhenyao Mo, 2014-04-08 21:13:55]

https://chromiumcodereview.appspot.com/228653004/diff/20001/gpu/command_buffe...
File gpu/command_buffer/client/query_tracker.cc (right):

https://chromiumcodereview.appspot.com/228653004/diff/20001/gpu/command_buffe...
gpu/command_buffer/client/query_tracker.cc:175: last_flush_ =
helper->flush_generation();
My apology for not catching this earlier, but this makes situation worse, not
better.  Now if you keep calling CheckResultsAvailable(), very likely
flush_generation == last_flush_, and you will flush everytime.

So instead, once you set last_flush_, you don't wanna update its value here. 
last_flush_ is also a misleading name, maybe just flush_count_.

[rptr, 2014-04-08 22:44:16]

PTAL.

https://chromiumcodereview.appspot.com/228653004/diff/20001/gpu/command_buffe...
File gpu/command_buffer/client/query_tracker.cc (right):

https://chromiumcodereview.appspot.com/228653004/diff/20001/gpu/command_buffe...
gpu/command_buffer/client/query_tracker.cc:175: last_flush_ =
helper->flush_generation();
On 2014/04/08 21:13:56, Zhenyao Mo wrote:
> My apology for not catching this earlier, but this makes situation worse, not
> better.  Now if you keep calling CheckResultsAvailable(), very likely
> flush_generation == last_flush_, and you will flush everytime.
> 
> So instead, once you set last_flush_, you don't wanna update its value here. 
> last_flush_ is also a misleading name, maybe just flush_count_.

Done.
Problem was updating flush_count_ here. we should update that only when EndQuery
is called.

[piman, 2014-04-08 22:55:39]

https://chromiumcodereview.appspot.com/228653004/diff/40001/gpu/command_buffe...
File gpu/command_buffer/client/query_tracker.cc (right):

https://chromiumcodereview.appspot.com/228653004/diff/40001/gpu/command_buffe...
gpu/command_buffer/client/query_tracker.cc:172: if
(!(helper->flush_generation()>flush_count_)) {
nit: spaces around '>'. Also, if (!(a>b)) can be written more clearly if (a <=
b)

https://chromiumcodereview.appspot.com/228653004/diff/40001/gpu/command_buffe...
gpu/command_buffer/client/query_tracker.cc:172: if
(!(helper->flush_generation()>flush_count_)) {
Could we handle wraparound of the generation here? Assuming we don't have 2^31
flushes outstanding (which is not possible), this should work:

if ((helper->flush_generation() - flush_count_ - 1) >= 0x80000000) {
  Flush();
} else {
  Noop();
}

[rptr, 2014-04-09 10:42:04]

please take a look.

https://chromiumcodereview.appspot.com/228653004/diff/40001/gpu/command_buffe...
File gpu/command_buffer/client/query_tracker.cc (right):

https://chromiumcodereview.appspot.com/228653004/diff/40001/gpu/command_buffe...
gpu/command_buffer/client/query_tracker.cc:172: if
(!(helper->flush_generation()>flush_count_)) {
On 2014/04/08 22:55:40, piman wrote:
> Could we handle wraparound of the generation here? Assuming we don't have 2^31
> flushes outstanding (which is not possible), this should work:
> 
> if ((helper->flush_generation() - flush_count_ - 1) >= 0x80000000) {
>   Flush();
> } else {
>   Noop();
> }

Done.

https://chromiumcodereview.appspot.com/228653004/diff/40001/gpu/command_buffe...
gpu/command_buffer/client/query_tracker.cc:172: if
(!(helper->flush_generation()>flush_count_)) {
On 2014/04/08 22:55:40, piman wrote:
> nit: spaces around '>'. Also, if (!(a>b)) can be written more clearly if (a <=
> b)

nit: sorry for that, forgot to run cl format before upload.

And regarding !(a>b) i thought this way its more clear while reading code as
flush_gen is never going to be less than flush_count recorded during EndQuery. 
Anyways new wraparound change takes care of this.

[piman, 2014-04-09 17:02:01]

Ok, could you write a unit test for this?

[rptr, 2014-04-12 15:08:41]

Hi Mo, piman,

Added a testcase to check for avoiding extra flushes between a call to EndQuery
& CheckResultAvailable.
Also added a case to check repeated CheckResultsAvailable calls wont perform
unnecessary flushes.
PTAL.

But i have a problem in one of gpu_unittests,
GLES2ImplementationTest.BeginEndQueryEXT 
This testcase checks for some invalid query-begin/end combinations and tests
some invalid queries.
It uses GetGLError() calls for failure cases and results in additional flush
calls (GetGLError=>WaitForCmd=>Finish=>Flush()..)

Between EndQuery command and GetQueryObject*() 3 flushes executed because of
GetGLError calls in this testcase.

At the end testcase also checks that none of the commands are left in ringbuffer

(NoCommandsWritten()) when testcase compeltes.
  ClearCommands();
  gl_->GetQueryObjectuivEXT(id1, GL_QUERY_RESULT_AVAILABLE_EXT, &available);
  EXPECT_TRUE(NoCommandsWritten());

But In CheckForAvailableStatus we add a No-op call to ring-buffer instead of
flush() as many flushes already went through.
NoCommandsWritten() will fail now because of no-op.
Earlier code would have happily flushed once more and test case would have
passed :). 
We should avoid this extra flush. Also we cant avoid no-op: its required to know
if GPU process is still alive.
Any suggestions for this failing testcase will be helpful.

[piman, 2014-04-14 19:31:51]

LGTM.
I think it's ok to remove that failing expectation, I don't think it's
meaningful for the test.

https://chromiumcodereview.appspot.com/228653004/diff/80001/gpu/command_buffe...
File gpu/command_buffer/client/query_tracker_unittest.cc (right):

https://chromiumcodereview.appspot.com/228653004/diff/80001/gpu/command_buffe...
gpu/command_buffer/client/query_tracker_unittest.cc:171: uint32 gen1, gen2,
gen3, gen4;
nit: declare each of them on their first use below

[rptr, 2014-04-15 16:54:21]

Please take a look. 

I'm now in CLA as confirmed from Laszlo through mail today. Could you please
push this first patch if its fine.
updated authors file for the same.
(/proc/cpuinfo fix is waiting two more owners approve, so ill remove authors
file from that fix).

https://chromiumcodereview.appspot.com/228653004/diff/80001/gpu/command_buffe...
File gpu/command_buffer/client/query_tracker_unittest.cc (right):

https://chromiumcodereview.appspot.com/228653004/diff/80001/gpu/command_buffe...
gpu/command_buffer/client/query_tracker_unittest.cc:171: uint32 gen1, gen2,
gen3, gen4;
On 2014/04/14 19:31:51, piman wrote:
> nit: declare each of them on their first use below

Done.

[Zhenyao Mo, 2014-04-15 17:04:29]

LGTM

https://chromiumcodereview.appspot.com/228653004/diff/100001/gpu/command_buff...
File gpu/command_buffer/client/query_tracker_unittest.cc (right):

https://chromiumcodereview.appspot.com/228653004/diff/100001/gpu/command_buff...
gpu/command_buffer/client/query_tracker_unittest.cc:110: uint32
GetHelperFlushGeneration() { return helper_->flush_generation(); }
nit: this should just be GetFlushGeneration().

[rptr, 2014-04-15 17:16:00]

https://chromiumcodereview.appspot.com/228653004/diff/100001/gpu/command_buff...
File gpu/command_buffer/client/query_tracker_unittest.cc (right):

https://chromiumcodereview.appspot.com/228653004/diff/100001/gpu/command_buff...
gpu/command_buffer/client/query_tracker_unittest.cc:110: uint32
GetHelperFlushGeneration() { return helper_->flush_generation(); }
On 2014/04/15 17:04:29, Zhenyao Mo wrote:
> nit: this should just be GetFlushGeneration().

Done.

[rptr, 2014-04-15 17:30:06]

The CQ bit was checked by puttaraju.r@samsung.com

[commit-bot: I haz the power, 2014-04-15 17:30:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/puttaraju.r@samsung.com/228653004/140001

[commit-bot: I haz the power, 2014-04-16 01:20:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/puttaraju.r@samsung.com/228653004/140001

[commit-bot: I haz the power, 2014-04-16 01:45:36]

Change committed as 264057