Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(264)

Issue 1016563004: Statistical stack profiler for Windows x64 (Closed)

Created:
5 years, 9 months ago by Mike Wittman
Modified:
5 years, 9 months ago
CC:
chromium-reviews, erikwright+watch_chromium.org, danduong, Alexei Svitkine (slow), Ilya Sherman
Base URL:
https://chromium.googlesource.com/chromium/src.git@lkcr
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Statistical stack profiler for Windows x64 Provides the collection part of a client stack collection pipeline for Win x64. This is intended to be used initially to provide real-world stack trace data to UMA to facilitate implementation of server-side processing. This CL has just the profiler implementation. Instantiation and use of the profiler in Chrome, and handling by UMA, will be addressed in follow-on CLs. BUG=464929 Committed: https://crrev.com/b450e197e878aec522e7299004dba511c65f254c Cr-Commit-Position: refs/heads/master@{#321658}

Patch Set 1 #

Total comments: 29

Patch Set 2 : address comments #

Patch Set 3 : record IPs for invalid modules #

Total comments: 40

Patch Set 4 : address comments #

Total comments: 2

Patch Set 5 : address comments #

Total comments: 2

Patch Set 6 : add diagnostic output for test failure #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1140 lines, -0 lines) Patch
M base/BUILD.gn View 2 chunks +5 lines, -0 lines 0 comments Download
M base/base.gyp View 1 chunk +1 line, -0 lines 0 comments Download
M base/base.gypi View 1 chunk +4 lines, -0 lines 0 comments Download
A base/profiler/stack_sampling_profiler.h View 1 1 chunk +206 lines, -0 lines 0 comments Download
A base/profiler/stack_sampling_profiler.cc View 1 2 3 4 1 chunk +261 lines, -0 lines 0 comments Download
A base/profiler/stack_sampling_profiler_posix.cc View 1 1 chunk +14 lines, -0 lines 0 comments Download
A base/profiler/stack_sampling_profiler_unittest.cc View 1 2 3 4 5 1 chunk +324 lines, -0 lines 0 comments Download
A base/profiler/stack_sampling_profiler_win.cc View 1 2 3 4 1 chunk +325 lines, -0 lines 0 comments Download

Messages

Total messages: 24 (6 generated)
Mike Wittman
PTAL This CL is split off from the metrics provider in https://crrev.com/981143006, and I've added ...
5 years, 9 months ago (2015-03-16 23:58:26 UTC) #2
Alexei Svitkine (slow)
https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_sampling_profiler.cc File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_sampling_profiler.cc#newcode20 base/profiler/stack_sampling_profiler.cc:20: namespace { Add a blank line after this. https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_sampling_profiler.cc#newcode24 ...
5 years, 9 months ago (2015-03-17 13:10:00 UTC) #4
Mike Wittman
https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_sampling_profiler.cc File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_sampling_profiler.cc#newcode20 base/profiler/stack_sampling_profiler.cc:20: namespace { On 2015/03/17 13:10:00, Alexei Svitkine (away) wrote: ...
5 years, 9 months ago (2015-03-17 19:07:38 UTC) #5
Mike Wittman
https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_sampling_profiler_win.cc File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_sampling_profiler_win.cc#newcode237 base/profiler/stack_sampling_profiler_win.cc:237: if (!modules[i]) On 2015/03/17 19:07:38, Mike Wittman wrote: > ...
5 years, 9 months ago (2015-03-18 19:17:22 UTC) #6
zturner
Mostly commenting on the Windows stuff. I'll leave the posix stuff for others. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sampling_profiler.h File ...
5 years, 9 months ago (2015-03-18 23:04:37 UTC) #8
danduong
https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sampling_profiler.h File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sampling_profiler.h#newcode66 base/profiler/stack_sampling_profiler.h:66: // GUID + AGE in the debug image headers ...
5 years, 9 months ago (2015-03-18 23:20:48 UTC) #10
Mike Wittman
https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sampling_profiler.h File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sampling_profiler.h#newcode85 base/profiler/stack_sampling_profiler.h:85: using Sample = std::vector<Frame>; On 2015/03/18 23:04:36, zturner wrote: ...
5 years, 9 months ago (2015-03-19 00:15:45 UTC) #11
cpu_(ooo_6.6-7.5)
https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sampling_profiler.cc File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sampling_profiler.cc#newcode195 base/profiler/stack_sampling_profiler.cc:195: delete thread; why do we need a special deleter? ...
5 years, 9 months ago (2015-03-19 02:49:04 UTC) #12
zturner
https://codereview.chromium.org/1016563004/diff/60001/base/profiler/stack_sampling_profiler_win.cc File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1016563004/diff/60001/base/profiler/stack_sampling_profiler_win.cc#newcode148 base/profiler/stack_sampling_profiler_win.cc:148: if (::ResumeThread(thread_handle) == -1) One minor comment here. This ...
5 years, 9 months ago (2015-03-19 02:54:12 UTC) #13
danduong
https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sampling_profiler.h File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sampling_profiler.h#newcode69 base/profiler/stack_sampling_profiler.h:69: FilePath filename; On 2015/03/19 02:49:04, cpu wrote: > are ...
5 years, 9 months ago (2015-03-19 03:09:45 UTC) #14
Mike Wittman
https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sampling_profiler.cc File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sampling_profiler.cc#newcode195 base/profiler/stack_sampling_profiler.cc:195: delete thread; On 2015/03/19 02:49:04, cpu wrote: > why ...
5 years, 9 months ago (2015-03-19 20:27:17 UTC) #15
cpu_(ooo_6.6-7.5)
lgtm for the windows part, we need a base/ reviewer that understands the posix side. ...
5 years, 9 months ago (2015-03-20 19:34:41 UTC) #16
zturner
On 2015/03/20 19:34:41, cpu wrote: > lgtm for the windows part, we need a base/ ...
5 years, 9 months ago (2015-03-20 19:38:14 UTC) #17
Mike Wittman
On 2015/03/20 19:38:14, zturner wrote: > On 2015/03/20 19:34:41, cpu wrote: > > lgtm for ...
5 years, 9 months ago (2015-03-20 20:55:53 UTC) #18
Mike Wittman
https://codereview.chromium.org/1016563004/diff/80001/base/profiler/stack_sampling_profiler_unittest.cc File base/profiler/stack_sampling_profiler_unittest.cc (right): https://codereview.chromium.org/1016563004/diff/80001/base/profiler/stack_sampling_profiler_unittest.cc#newcode211 base/profiler/stack_sampling_profiler_unittest.cc:211: ASSERT_TRUE(loc != sample.end()) << "function not found in stack"; ...
5 years, 9 months ago (2015-03-20 20:56:20 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1016563004/100001
5 years, 9 months ago (2015-03-20 21:24:03 UTC) #22
commit-bot: I haz the power
Committed patchset #6 (id:100001)
5 years, 9 months ago (2015-03-20 22:52:37 UTC) #23
commit-bot: I haz the power
5 years, 9 months ago (2015-03-20 22:53:24 UTC) #24
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/b450e197e878aec522e7299004dba511c65f254c
Cr-Commit-Position: refs/heads/master@{#321658}

Powered by Google App Engine
This is Rietveld 408576698