Use TimeTicks instead of clock() in gpu::CommandBufferHelper

Use TimeTicks instead of clock() in gpu::CommandBufferHelper

clock() returns a processor time, which does not change while
a thread is sleeping. So, Flush() would be called less often
than you expected.

TimeTicks uses a monotonic timer so this would be a better
choice than Time because TimeTicks is not affected by host's
time settings.

BUG=264856
TEST=trybots

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

[hamaji, 2014-07-09 06:03:15]

I created a patch based on the email thread. What do you think?

I'm guessing we will call Flush more often than before with this patch. Is there
a benchmark you usually use for this kind of change?

https://codereview.chromium.org/381473004/diff/1/gpu/command_buffer/client/cm...
File gpu/command_buffer/client/cmd_buffer_helper.cc (right):

https://codereview.chromium.org/381473004/diff/1/gpu/command_buffer/client/cm...
gpu/command_buffer/client/cmd_buffer_helper.cc:10: #include "base/time/time.h"
I guessed using TimeTicks in this file is fine because we already include
base/logging.h. Correct me if I'm wrong.

https://codereview.chromium.org/381473004/diff/1/gpu/command_buffer/client/cm...
File gpu/command_buffer/client/cmd_buffer_helper.h (right):

https://codereview.chromium.org/381473004/diff/1/gpu/command_buffer/client/cm...
gpu/command_buffer/client/cmd_buffer_helper.h:328: // Using primitive type
instead of base::TimeTicks because this file
Is this comment right? It seems other header files in this directory #include
base headers.

For now, I declared this as int64 assuming this comment is right.

[piman, 2014-07-09 20:13:47]

https://codereview.chromium.org/381473004/diff/1/gpu/command_buffer/client/cm...
File gpu/command_buffer/client/cmd_buffer_helper.h (right):

https://codereview.chromium.org/381473004/diff/1/gpu/command_buffer/client/cm...
gpu/command_buffer/client/cmd_buffer_helper.h:328: // Using primitive type
instead of base::TimeTicks because this file
On 2014/07/09 06:03:15, hamaji wrote:
> Is this comment right? It seems other header files in this directory #include
> base headers.
> 
> For now, I declared this as int64 assuming this comment is right.

This used to be true because of NaCl, but now it should be ok to depend on base
(we've been progressively fixing headers to that effect).

[vmiura, 2014-07-09 20:18:59]

https://codereview.chromium.org/381473004/diff/1/gpu/command_buffer/client/cm...
File gpu/command_buffer/client/cmd_buffer_helper.cc (right):

https://codereview.chromium.org/381473004/diff/1/gpu/command_buffer/client/cm...
gpu/command_buffer/client/cmd_buffer_helper.cc:167:
base::TimeDelta::FromSecondsD(kPeriodicFlushDelay)) {
Could we change this to FromMicroseconds to avoid doubles -> fixed conversions?.

[hamaji, 2014-07-10 07:00:16]

BTW, I checked how often Flush is triggered by adding LOG(ERROR) and run a GL
heavy NaCl app. The number of Flush calls was not changed by this patch. Though
I admit this check is naive, I became a bit more confident that this patch won't
regress. That said, if there's a good way to check the performance of this patch
beforehand, I'll happily try it.

Thanks!

https://codereview.chromium.org/381473004/diff/1/gpu/command_buffer/client/cm...
File gpu/command_buffer/client/cmd_buffer_helper.cc (right):

https://codereview.chromium.org/381473004/diff/1/gpu/command_buffer/client/cm...
gpu/command_buffer/client/cmd_buffer_helper.cc:167:
base::TimeDelta::FromSecondsD(kPeriodicFlushDelay)) {
On 2014/07/09 20:18:58, vmiura wrote:
> Could we change this to FromMicroseconds to avoid doubles -> fixed
conversions?.

Did you worry about that 1.0/300 will be converted to 0? If so, it won't happen
as I was using FromSeconds*D*. Actually, the assembly code had 0xd05, which is
3333 = 1000*1000/300. The compiler eliminated the use of float completely.

That said, there would be no reason to keep the float value. I have updated the
patch.

https://codereview.chromium.org/381473004/diff/1/gpu/command_buffer/client/cm...
File gpu/command_buffer/client/cmd_buffer_helper.h (right):

https://codereview.chromium.org/381473004/diff/1/gpu/command_buffer/client/cm...
gpu/command_buffer/client/cmd_buffer_helper.h:328: // Using primitive type
instead of base::TimeTicks because this file
On 2014/07/09 20:13:46, piman (slow to review) wrote:
> On 2014/07/09 06:03:15, hamaji wrote:
> > Is this comment right? It seems other header files in this directory
#include
> > base headers.
> > 
> > For now, I declared this as int64 assuming this comment is right.
> 
> This used to be true because of NaCl, but now it should be ok to depend on
base
> (we've been progressively fixing headers to that effect).

Good news! Now I removed the comment and changed clock_t to TimeTicks.

[jbauman, 2014-07-10 22:12:46]

lgtm

I only used clock() originally because time() didn't have enough resolution and
base/ wasn't allowed in NaCl, so this should be fine now.

[vmiura, 2014-07-10 22:21:25]

lgtm

https://codereview.chromium.org/381473004/diff/1/gpu/command_buffer/client/cm...
File gpu/command_buffer/client/cmd_buffer_helper.cc (right):

https://codereview.chromium.org/381473004/diff/1/gpu/command_buffer/client/cm...
gpu/command_buffer/client/cmd_buffer_helper.cc:167:
base::TimeDelta::FromSecondsD(kPeriodicFlushDelay)) {
On 2014/07/10 07:00:16, hamaji wrote:
> On 2014/07/09 20:18:58, vmiura wrote:
> > Could we change this to FromMicroseconds to avoid doubles -> fixed
> conversions?.
> 
> Did you worry about that 1.0/300 will be converted to 0? If so, it won't
happen
> as I was using FromSeconds*D*. Actually, the assembly code had 0xd05, which is
> 3333 = 1000*1000/300. The compiler eliminated the use of float completely.
> 
> That said, there would be no reason to keep the float value. I have updated
the
> patch.

Ok, it was just a performance question.  In some targets inlining might be
different, e.g. Android compiled with -Os and ARM has bad float & double support
(although this code is currently disabled on Android but on for CrOS).

Thanks for changing.

[hamaji, 2014-07-11 06:10:57]

The CQ bit was checked by hamaji@chromium.org

[commit-bot: I haz the power, 2014-07-11 06:12:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hamaji@chromium.org/381473004/20001

[commit-bot: I haz the power, 2014-07-11 07:46:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-11 09:27:19]

Change committed as 282593