Windows-native sampling profiler wrapper class.

Windows-native sampling profiler wrapper class.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113473

[Sigurður Ásgeirsson, 2011-12-05 20:50:54]

This is the first-cut implementation of the sampling profiler wrapper class.
Please review the interface in preference to  what there is of implementation so
far.
If there's a more appropriate place to put this class, then please advise as
well.

[M-A Ruel, 2011-12-06 01:52:12]

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler.cc
File base/win/sampling_profiler.cc (right):

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler.cc#n...
base/win/sampling_profiler.cc:129: DCHECK(size != 0);
DCHECK_NE()?

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler_unit...
File base/win/sampling_profiler_unittest.cc (right):

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler_unit...
base/win/sampling_profiler_unittest.cc:70: FILETIME ignore, user_time = {};
style nit: prefer one line per variable.

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler_unit...
base/win/sampling_profiler_unittest.cc:75: base::TimeDelta spin_time =
base::TimeDelta::FromMilliseconds(500);
500 is a lot for a unit test, what about 20?

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler_unit...
base/win/sampling_profiler_unittest.cc:89: // Start the profiler.
??

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler_unit...
base/win/sampling_profiler_unittest.cc:95: 
two vertical spaces here? one would suffice.

[Jói, 2011-12-06 12:36:54]

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler.cc
File base/win/sampling_profiler.cc (right):

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler.cc#n...
base/win/sampling_profiler.cc:7: #include <winternl.h>  // for NTSTATUS.
space between include sections, i.e. after this line

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler.cc#n...
base/win/sampling_profiler.cc:150: while ((1 << log2_bucket_size) < bucket_size)
Here it occurs to me that you should simply take the power-of-two of the bucket
size, rather than a bucket size that must then be DCHECKed to be a power of two.

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler.h
File base/win/sampling_profiler.h (right):

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler.h#ne...
base/win/sampling_profiler.h:9: #include <vector>
blank line after this

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler.h#ne...
base/win/sampling_profiler.h:52: const std::vector<ULONG>& buckets() const {
return buckets_; }
Does this need a comment on when it is safe to read it?  I guess it should
always be OK?

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler_unit...
File base/win/sampling_profiler_unittest.cc (right):

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler_unit...
base/win/sampling_profiler_unittest.cc:52: };
};  ->  }  // namespace

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler_unit...
base/win/sampling_profiler_unittest.cc:75: base::TimeDelta spin_time =
base::TimeDelta::FromMilliseconds(500);
Use TestTimeouts (base/test_timeouts.h) for resilience when running under e.g.
Dr. Memory.

[Sigurður Ásgeirsson, 2011-12-06 14:46:02]

Thanks, please take another look.

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler.cc
File base/win/sampling_profiler.cc (right):

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler.cc#n...
base/win/sampling_profiler.cc:7: #include <winternl.h>  // for NTSTATUS.
On 2011/12/06 12:36:55, Jói wrote:
> space between include sections, i.e. after this line

Done.

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler.cc#n...
base/win/sampling_profiler.cc:129: DCHECK(size != 0);
On 2011/12/06 01:52:13, Marc-Antoine Ruel wrote:
> DCHECK_NE()?

Done.

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler.cc#n...
base/win/sampling_profiler.cc:150: while ((1 << log2_bucket_size) < bucket_size)
On 2011/12/06 12:36:55, Jói wrote:
> Here it occurs to me that you should simply take the power-of-two of the
bucket
> size, rather than a bucket size that must then be DCHECKed to be a power of
two.

Good point, done.

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler.h
File base/win/sampling_profiler.h (right):

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler.h#ne...
base/win/sampling_profiler.h:9: #include <vector>
On 2011/12/06 12:36:55, Jói wrote:
> blank line after this

Done.

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler.h#ne...
base/win/sampling_profiler.h:52: const std::vector<ULONG>& buckets() const {
return buckets_; }
On 2011/12/06 12:36:55, Jói wrote:
> Does this need a comment on when it is safe to read it?  I guess it should
> always be OK?

Done.

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler_unit...
File base/win/sampling_profiler_unittest.cc (right):

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler_unit...
base/win/sampling_profiler_unittest.cc:52: };
On 2011/12/06 12:36:55, Jói wrote:
> };  ->  }  // namespace

Done.

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler_unit...
base/win/sampling_profiler_unittest.cc:70: FILETIME ignore, user_time = {};
On 2011/12/06 01:52:13, Marc-Antoine Ruel wrote:
> style nit: prefer one line per variable.

Done.

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler_unit...
base/win/sampling_profiler_unittest.cc:75: base::TimeDelta spin_time =
base::TimeDelta::FromMilliseconds(500);
On 2011/12/06 12:36:55, Jói wrote:
> Use TestTimeouts (base/test_timeouts.h) for resilience when running under e.g.
> Dr. Memory.

Hmmm, that's a good point. The test here, as written, will fail under Dr.
Memory, as no samples will ever land in our bucket.

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler_unit...
base/win/sampling_profiler_unittest.cc:89: // Start the profiler.
On 2011/12/06 01:52:13, Marc-Antoine Ruel wrote:
> ??

Done.

http://codereview.chromium.org/8803022/diff/1/base/win/sampling_profiler_unit...
base/win/sampling_profiler_unittest.cc:95: 
On 2011/12/06 01:52:13, Marc-Antoine Ruel wrote:
> two vertical spaces here? one would suffice.

Done.

[Jói, 2011-12-06 14:58:16]

LGTM

[M-A Ruel, 2011-12-06 15:02:50]

lgtm

http://codereview.chromium.org/8803022/diff/6002/base/win/sampling_profiler.h
File base/win/sampling_profiler.h (right):

http://codereview.chromium.org/8803022/diff/6002/base/win/sampling_profiler.h...
base/win/sampling_profiler.h:43: bool Start();
For functions like that, I always ask myself: is the return value really useful?
Can the client do anything about it or should it care?

Personally I'd prefer silent failure for these kinds of things, since it's not a
fatal failure and the app can't do much about it. So Stop() could be called even
if Start() failed. The client shouldn't care about it.

But I don't really mind so ignore the comment above. :)

[Sigurður Ásgeirsson, 2011-12-06 16:12:08]

Thanks. Committing.

http://codereview.chromium.org/8803022/diff/6002/base/win/sampling_profiler.h
File base/win/sampling_profiler.h (right):

http://codereview.chromium.org/8803022/diff/6002/base/win/sampling_profiler.h...
base/win/sampling_profiler.h:43: bool Start();
On 2011/12/06 15:02:50, Marc-Antoine Ruel wrote:
> For functions like that, I always ask myself: is the return value really
useful?
> Can the client do anything about it or should it care?
> 
> Personally I'd prefer silent failure for these kinds of things, since it's not
a
> fatal failure and the app can't do much about it. So Stop() could be called
even
> if Start() failed. The client shouldn't care about it.
> 
> But I don't really mind so ignore the comment above. :)

That's good comment. I started changing the interface, but then that leaves me
with a CHECK, as if a profile can't be stopped, you'll end up with a random
memory overwrite in the future as the vector is freed ...
I'm pretty sure stopping a profile will never fail, so maybe this is a
non-issue...

[commit-bot: I haz the power, 2011-12-06 16:13:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/siggi@chromium.org/8803022/6002

[Sigurður Ásgeirsson, 2011-12-06 16:15:03]

On Tue, Dec 6, 2011 at 11:12, <siggi@chromium.org> wrote:

> Thanks. Committing.
>
>
>
> http://codereview.chromium.**org/8803022/diff/6002/base/**
>
win/sampling_profiler.h<http://codereview.chromium.org/8803022/diff/6002/base/win/sampling_profiler.h>
> File base/win/sampling_profiler.h (right):
>
> http://codereview.chromium.**org/8803022/diff/6002/base/**
>
win/sampling_profiler.h#**newcode43<http://codereview.chromium.org/8803022/diff/6002/base/win/sampling_profiler.h#newcode43>
> base/win/sampling_profiler.h:**43: bool Start();
> On 2011/12/06 15:02:50, Marc-Antoine Ruel wrote:
>
>> For functions like that, I always ask myself: is the return value
>>
> really useful?
>
>> Can the client do anything about it or should it care?
>>
>
>  Personally I'd prefer silent failure for these kinds of things, since
>>
> it's not a
>
>> fatal failure and the app can't do much about it. So Stop() could be
>>
> called even
>
>> if Start() failed. The client shouldn't care about it.
>>
>
>  But I don't really mind so ignore the comment above. :)
>>
>
> That's good comment. I started changing the interface, but then that
> leaves me with a CHECK, as if a profile can't be stopped, you'll end up
> with a random memory overwrite in the future as the vector is freed ...
> I'm pretty sure stopping a profile will never fail, so maybe this is a
> non-issue...
>
>
http://codereview.chromium.**org/8803022/<http://codereview.chromium.org/8803...
>

[commit-bot: I haz the power, 2011-12-06 16:29:07]

Try job failure for 8803022-6002 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-12-06 16:42:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/siggi@chromium.org/8803022/9001

[commit-bot: I haz the power, 2011-12-06 17:43:45]

Try job failure for 8803022-9001 (retry) on linux_rel for step "check_deps".
It's a second try, previously, step "check_deps" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-12-06 17:49:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/siggi@chromium.org/8803022/11001

[commit-bot: I haz the power, 2011-12-06 19:45:58]

Try job failure for 8803022-11001 (retry) on win_rel for step "base_unittests".
It's a second try, previously, step "base_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-12-06 19:46:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/siggi@chromium.org/8803022/11001

[commit-bot: I haz the power, 2011-12-07 19:11:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/siggi@chromium.org/8803022/19001

[commit-bot: I haz the power, 2011-12-07 19:24:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/siggi@chromium.org/8803022/19001

[commit-bot: I haz the power, 2011-12-07 21:44:31]

Change committed as 113473

[Timur Iskhodzhanov, 2011-12-08 09:48:14]

Fails on TSan and DrMemory bots
http://code.google.com/p/chromium/issues/detail?id=106829