gpu: Extract the gpu timing code from gpu_tracer.

gpu: Extract the gpu timing code from gpu_tracer.

This commit introduces two new classes: GPUTiming and GPUTimer. The first one takes care of checking which timing GL extension to use, and the second one measures the amount of time it takes to complete a set of opengl commands.
GPUTracer and GPUTrace have been modified accordingly to use the two new classes.

BUG=

Committed: https://crrev.com/82922f87f213ee34c2dc319b937c5137a939ac52
Cr-Commit-Position: refs/heads/master@{#316020}

[Daniele Castagna, 2015-02-05 22:16:18]

dcastagna@chromium.org changed reviewers:
+ dyen@chromium.org

[David Yen, 2015-02-06 22:47:00]

dyen@chromium.org changed reviewers:
+ vmiura@chromium.org

[David Yen, 2015-02-06 22:47:01]

+vmiura

Overall it looks like it's going in the right direction, great job. Device
traces still do not work correctly though, and I did not see anything obviously
wrong through my review. That will need to be looked into more.

https://codereview.chromium.org/904743002/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_timing.cc (right):

https://codereview.chromium.org/904743002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_timing.cc:79: CheckAndResetTimerErrors();
The ResetTimerErrors() needs to be called everytime decoding begins not during
initialization.

https://codereview.chromium.org/904743002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_timing.cc:83: offset_ = CalculateTimerOffset();
I think we don't need to call CalculateTimerOffset() here, just depend on the
first time Start() calls it.

https://codereview.chromium.org/904743002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_timing.cc:106: DCHECK_EQ(kTimerTypeDisjoint,
timer_type_);
This DCHECK seems unnecessary, was it from old code?

https://codereview.chromium.org/904743002/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/904743002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.cc:157: // Begin a Trace for all active
markers
gpu_timing_ needs to reset the disjoint state here.

https://codereview.chromium.org/904743002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.cc:246: return (*gpu_trace_srv_category !=
0) || (*gpu_trace_dev_category != '\0');
I don't really mind which but these 2 should probably be the same.

https://codereview.chromium.org/904743002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.cc:302: traces_.clear();
indentation needs to be corrected now.

https://codereview.chromium.org/904743002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.cc:322:
gpu_timing_.CalculateTimerOffset();
Since gpu_timing now internally keeps track of when the timer offset needs to be
recalculated, we can probably just defer this to when traces begin and take out
this function completely. However we should still invalidate the timer offset on
BeginDecoding() when tracing is turned off. We can just delete this function and
move the invalidation to be inside of BeginDecoding().

[Daniele Castagna, 2015-02-10 04:49:33]

Thank you for the review!

The problem with the device traces not working correctly was a useless float
conversion that lost precision for big numbers. This explains also why tests
were passing even though the timings in chrome://tracing were off.

https://codereview.chromium.org/904743002/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_timing.cc (right):

https://codereview.chromium.org/904743002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_timing.cc:79: CheckAndResetTimerErrors();
On 2015/02/06 22:47:01, David Yen wrote:
> The ResetTimerErrors() needs to be called everytime decoding begins not during
> initialization.

Done.

https://codereview.chromium.org/904743002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_timing.cc:83: offset_ = CalculateTimerOffset();
On 2015/02/06 22:47:01, David Yen wrote:
> I think we don't need to call CalculateTimerOffset() here, just depend on the
> first time Start() calls it.

Done.

https://codereview.chromium.org/904743002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_timing.cc:106: DCHECK_EQ(kTimerTypeDisjoint,
timer_type_);
On 2015/02/06 22:47:01, David Yen wrote:
> This DCHECK seems unnecessary, was it from old code?

Done.

https://codereview.chromium.org/904743002/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/904743002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.cc:157: // Begin a Trace for all active
markers
On 2015/02/06 22:47:01, David Yen wrote:
> gpu_timing_ needs to reset the disjoint state here.

Done.

https://codereview.chromium.org/904743002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.cc:246: return (*gpu_trace_srv_category !=
0) || (*gpu_trace_dev_category != '\0');
On 2015/02/06 22:47:01, David Yen wrote:
> I don't really mind which but these 2 should probably be the same.

Done.

https://codereview.chromium.org/904743002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.cc:302: traces_.clear();
On 2015/02/06 22:47:01, David Yen wrote:
> indentation needs to be corrected now.

I'm using git-clang-format, isn't that enough?

https://codereview.chromium.org/904743002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.cc:322:
gpu_timing_.CalculateTimerOffset();
On 2015/02/06 22:47:01, David Yen wrote:
> Since gpu_timing now internally keeps track of when the timer offset needs to
be
> recalculated, we can probably just defer this to when traces begin and take
out
> this function completely. However we should still invalidate the timer offset
on
> BeginDecoding() when tracing is turned off. We can just delete this function
and
> move the invalidation to be inside of BeginDecoding().

Done.

[vmiura, 2015-02-10 07:43:14]

https://codereview.chromium.org/904743002/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/904743002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.cc:302: traces_.clear();
On 2015/02/10 04:49:33, dcastagna wrote:
> On 2015/02/06 22:47:01, David Yen wrote:
> > indentation needs to be corrected now.
> 
> I'm using git-clang-format, isn't that enough?

There should be a 2 space indentation.  Did you run "git cl format"?

[Daniele Castagna, 2015-02-10 17:28:27]

https://codereview.chromium.org/904743002/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/904743002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.cc:302: traces_.clear();
On 2015/02/10 07:43:13, vmiura wrote:
> On 2015/02/10 04:49:33, dcastagna wrote:
> > On 2015/02/06 22:47:01, David Yen wrote:
> > > indentation needs to be corrected now.
> > 
> > I'm using git-clang-format, isn't that enough?
> 
> There should be a 2 space indentation.  Did you run "git cl format"?

Is it possible that according to the diff that line didn't change and format
wouldn't touch it?

Fixed it manually.

[David Yen, 2015-02-10 22:30:01]

lgtm with global variable reset in unittest.

Wait for vmiura as he is the owner for the gpu tracer.

https://codereview.chromium.org/904743002/diff/60001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer_unittest.cc (right):

https://codereview.chromium.org/904743002/diff/60001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer_unittest.cc:28: int64 g_fakeCPUTime = 0;
Since this is a global variable, should it be reset during each SetUp()
function?

[Daniele Castagna, 2015-02-10 22:41:16]

New patchsets have been uploaded after l-g-t-m from dyen@chromium.org

[Daniele Castagna, 2015-02-10 22:42:36]

https://codereview.chromium.org/904743002/diff/60001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer_unittest.cc (right):

https://codereview.chromium.org/904743002/diff/60001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer_unittest.cc:28: int64 g_fakeCPUTime = 0;
On 2015/02/10 22:30:01, David Yen wrote:
> Since this is a global variable, should it be reset during each SetUp()
> function?

Done.

[vmiura, 2015-02-12 06:57:30]

LGTM, nice work.

[Daniele Castagna, 2015-02-12 07:07:03]

The CQ bit was checked by dcastagna@chromium.org

[commit-bot: I haz the power, 2015-02-12 07:07:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/904743002/80001

[commit-bot: I haz the power, 2015-02-12 07:12:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-12 07:12:58]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[vmiura, 2015-02-12 07:27:36]

vmiura@chromium.org changed reviewers:
+ sievers@chromium.org

[vmiura, 2015-02-12 07:27:36]

sievers@chromium.org: Please review changes in gypi file.

[no sievers, 2015-02-12 19:11:51]

On 2015/02/12 07:27:36, vmiura wrote:
> mailto:sievers@chromium.org: Please review changes in gypi file.

lgtm

[Daniele Castagna, 2015-02-12 19:13:09]

The CQ bit was checked by dcastagna@chromium.org

[commit-bot: I haz the power, 2015-02-12 19:13:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/904743002/80001

[commit-bot: I haz the power, 2015-02-12 19:21:39]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-12 19:22:56]

Patchset 5 (id:??) landed as
https://crrev.com/82922f87f213ee34c2dc319b937c5137a939ac52
Cr-Commit-Position: refs/heads/master@{#316020}

[Nico, 2015-02-13 05:56:00]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-02-13 05:56:01]

This leads to these reports on the DrMemory bots:

UNINITIALIZED READ: reading 0x0015e9bc-0x0015e9bd 1 byte(s) within
0x0015e9bc-0x0015e9c0
# 0 gpu.dll!gpu::GPUTiming::CheckAndResetTimerErrors                          
[gpu\command_buffer\service\gpu_timing.cc:107]
# 1 gpu.dll!gpu::gles2::GPUTracer::BeginDecoding                              
[gpu\command_buffer\service\gpu_tracer.cc:162]
# 2 gpu::gles2::`anonymous namespace'::BaseGpuTracerTest::DoDisjointTest      
[gpu\command_buffer\service\gpu_tracer_unittest.cc:533]
# 3 testing::internal::HandleExceptionsInMethodIfSupported<>                  
[testing\gtest\src\gtest.cc:2420]
Note: @0:00:32.043 in thread 3720
Note: instruction: cmp    0xfffffffc(%ebp) %eax
Suppression (error hash=#63E17FFDBA588F6C#):
For more info on using suppressions see
http://dev.chromium.org/developers/how-tos/using-drmemory#TOC-Suppressing-err...
{
UNINITIALIZED READ
name=<insert_a_suppression_name_here>
gpu.dll!gpu::GPUTiming::CheckAndResetTimerErrors
gpu.dll!gpu::gles2::GPUTracer::BeginDecoding
*!gpu::gles2::`anonymous namespace'::BaseGpuTracerTest::DoDisjointTest
*!testing::internal::HandleExceptionsInMethodIfSupported<>
}


It looks like timer_type_ isn't set in the constructor – it should probably be
set to some default value.

[Daniele Castagna, 2015-02-13 18:57:22]

On 2015/02/13 at 05:56:01, thakis wrote:
> This leads to these reports on the DrMemory bots:
> 
> UNINITIALIZED READ: reading 0x0015e9bc-0x0015e9bd 1 byte(s) within
0x0015e9bc-0x0015e9c0
> # 0 gpu.dll!gpu::GPUTiming::CheckAndResetTimerErrors                          
[gpu\command_buffer\service\gpu_timing.cc:107]
> # 1 gpu.dll!gpu::gles2::GPUTracer::BeginDecoding                              
[gpu\command_buffer\service\gpu_tracer.cc:162]
> # 2 gpu::gles2::`anonymous namespace'::BaseGpuTracerTest::DoDisjointTest      
[gpu\command_buffer\service\gpu_tracer_unittest.cc:533]
> # 3 testing::internal::HandleExceptionsInMethodIfSupported<>                  
[testing\gtest\src\gtest.cc:2420]
> Note: @0:00:32.043 in thread 3720
> Note: instruction: cmp    0xfffffffc(%ebp) %eax
> Suppression (error hash=#63E17FFDBA588F6C#):
> For more info on using suppressions see
http://dev.chromium.org/developers/how-tos/using-drmemory#TOC-Suppressing-err...
> {
> UNINITIALIZED READ
> name=<insert_a_suppression_name_here>
> gpu.dll!gpu::GPUTiming::CheckAndResetTimerErrors
> gpu.dll!gpu::gles2::GPUTracer::BeginDecoding
> *!gpu::gles2::`anonymous namespace'::BaseGpuTracerTest::DoDisjointTest
> *!testing::internal::HandleExceptionsInMethodIfSupported<>
> }
> 
> 
> It looks like timer_type_ isn't set in the constructor – it should probably be
set to some default value.

After a quick chat with thakis@ we determined that timer_type_ should be already
initialized correctly where it's defined. Non-Static Class Member Initializers
are allowed according to https://chromium-cpp.appspot.com/.
thakis@ is still looking into this.

[Nico, 2015-02-13 19:09:34]

Looking at this again, it seems I misread which line the report refers to.
Apparently it's about `return disjoing_value != 0`, which is even more
surprising to me. I filed
https://code.google.com/p/chromium/issues/detail?id=458607 for this.

On Fri, Feb 13, 2015 at 10:57 AM, <dcastagna@chromium.org> wrote:

> On 2015/02/13 at 05:56:01, thakis wrote:
>
>> This leads to these reports on the DrMemory bots:
>>
>
>  UNINITIALIZED READ: reading 0x0015e9bc-0x0015e9bd 1 byte(s) within
>>
> 0x0015e9bc-0x0015e9c0
>
>> # 0 gpu.dll!gpu::GPUTiming::CheckAndResetTimerErrors
>>
> [gpu\command_buffer\service\gpu_timing.cc:107]
>
>> # 1 gpu.dll!gpu::gles2::GPUTracer::BeginDecoding
>>
> [gpu\command_buffer\service\gpu_tracer.cc:162]
>
>> # 2 gpu::gles2::`anonymous namespace'::BaseGpuTracerTest::DoDisjointTest
>>
> [gpu\command_buffer\service\gpu_tracer_unittest.cc:533]
>
>> # 3 testing::internal::HandleExceptionsInMethodIfSupported<>
>>
> [testing\gtest\src\gtest.cc:2420]
>
>> Note: @0:00:32.043 in thread 3720
>> Note: instruction: cmp    0xfffffffc(%ebp) %eax
>> Suppression (error hash=#63E17FFDBA588F6C#):
>> For more info on using suppressions see
>>
> http://dev.chromium.org/developers/how-tos/using-drmemory#TOC-Suppressing-
> error-reports-from-the-
>
>> {
>> UNINITIALIZED READ
>> name=<insert_a_suppression_name_here>
>> gpu.dll!gpu::GPUTiming::CheckAndResetTimerErrors
>> gpu.dll!gpu::gles2::GPUTracer::BeginDecoding
>> *!gpu::gles2::`anonymous namespace'::BaseGpuTracerTest::DoDisjointTest
>> *!testing::internal::HandleExceptionsInMethodIfSupported<>
>> }
>>
>
>
>  It looks like timer_type_ isn't set in the constructor – it should
>> probably be
>>
> set to some default value.
>
> After a quick chat with thakis@ we determined that timer_type_ should be
> already
> initialized correctly where it's defined. Non-Static Class Member
> Initializers
> are allowed according to https://chromium-cpp.appspot.com/.
> thakis@ is still looking into this.
>
> https://codereview.chromium.org/904743002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.