Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(13)

Issue 2702463003: NativeStackSampler implementation for Mac. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months ago by Avi (ping after 24h)
Modified:
2 months, 3 weeks ago
CC:
chromium-reviews, mac-reviews_chromium.org, vmpstr+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

NativeStackSampler implementation for Mac. Stack sampling on Mac implemented for StackSamplingProfiler to provide Chrome stack trace data from Mac users to UMA. BUG=531673 Review-Url: https://codereview.chromium.org/2702463003 Cr-Commit-Position: refs/heads/master@{#461145} Committed: https://chromium.googlesource.com/chromium/src/+/965a47b7737c6f15238447d8d8fb7105183acc8a

Patch Set 1 #

Patch Set 2 : rev #

Patch Set 3 : 0u #

Total comments: 19

Patch Set 4 : Mike #

Patch Set 5 : fix #

Total comments: 25

Patch Set 6 : mark #

Patch Set 7 : config #

Patch Set 8 : fix for test #

Patch Set 9 : rev 2 #

Patch Set 10 : ios fix? #

Patch Set 11 : guess not re ios #

Total comments: 10

Patch Set 12 : mike #

Total comments: 2

Patch Set 13 : uintptr_t #

Patch Set 14 : ios build fix; don't run tests on ios #

Total comments: 54

Patch Set 15 : mark/robert #

Patch Set 16 : fix #

Patch Set 17 : fix config #

Total comments: 6

Patch Set 18 : rev #

Total comments: 7

Patch Set 19 : rev #

Patch Set 20 : . #

Total comments: 8

Patch Set 21 : rev #

Unified diffs Side-by-side diffs Delta from patch set Stats (+517 lines, -25 lines) Patch
M base/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 5 chunks +11 lines, -1 line 0 comments Download
A base/profiler/native_stack_sampler_mac.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +471 lines, -0 lines 0 comments Download
M base/profiler/native_stack_sampler_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +6 lines, -11 lines 0 comments Download
M base/profiler/stack_sampling_profiler_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 6 chunks +10 lines, -5 lines 0 comments Download
M chrome/common/stack_sampling_configuration.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +19 lines, -8 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 181 (116 generated)
Avi (ping after 24h)
Mike: For integration into the stack sampler world. There are a few cleanups to the ...
4 months ago (2017-02-16 06:11:16 UTC) #18
Mike Wittman
https://codereview.chromium.org/2702463003/diff/40001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2702463003/diff/40001/base/BUILD.gn#newcode2317 base/BUILD.gn:2317: data_deps += [ ":base_profiler_test_support_library" ] On 2017/02/16 06:11:15, Avi ...
4 months ago (2017-02-16 21:51:34 UTC) #21
Avi (ping after 24h)
Well, the trybots are pretty broken, but here you go for a new version. https://codereview.chromium.org/2702463003/diff/40001/base/BUILD.gn ...
4 months ago (2017-02-17 03:41:09 UTC) #48
Mark Mentovai
This is only partial. https://codereview.chromium.org/2702463003/diff/40001/base/profiler/native_stack_sampler_mac.cc File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2702463003/diff/40001/base/profiler/native_stack_sampler_mac.cc#newcode156 base/profiler/native_stack_sampler_mac.cc:156: step_result = unw_step(&unwind_cursor); Avi wrote: ...
4 months ago (2017-02-17 05:21:05 UTC) #52
Mark Mentovai
(my review, I mean.)
4 months ago (2017-02-17 05:21:31 UTC) #54
Mike Wittman
https://codereview.chromium.org/2702463003/diff/40001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2702463003/diff/40001/base/BUILD.gn#newcode2317 base/BUILD.gn:2317: data_deps += [ ":base_profiler_test_support_library" ] On 2017/02/17 03:41:09, Avi ...
4 months ago (2017-02-17 17:09:15 UTC) #59
Avi (ping after 24h)
https://codereview.chromium.org/2702463003/diff/40001/base/profiler/native_stack_sampler_mac.cc File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2702463003/diff/40001/base/profiler/native_stack_sampler_mac.cc#newcode156 base/profiler/native_stack_sampler_mac.cc:156: step_result = unw_step(&unwind_cursor); On 2017/02/17 17:09:15, Mike Wittman wrote: ...
4 months ago (2017-02-17 17:18:13 UTC) #60
Mike Wittman
https://codereview.chromium.org/2702463003/diff/40001/base/profiler/native_stack_sampler_mac.cc File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2702463003/diff/40001/base/profiler/native_stack_sampler_mac.cc#newcode156 base/profiler/native_stack_sampler_mac.cc:156: step_result = unw_step(&unwind_cursor); On 2017/02/17 17:18:12, Avi wrote: > ...
4 months ago (2017-02-17 17:38:51 UTC) #63
Avi (ping after 24h)
https://codereview.chromium.org/2702463003/diff/80001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2702463003/diff/80001/base/BUILD.gn#newcode1828 base/BUILD.gn:1828: shared_library("base_profiler_test_support_library") { On 2017/02/17 17:18:12, Avi wrote: > On ...
4 months ago (2017-02-17 17:47:40 UTC) #66
Mark Mentovai
Avi wrote: > https://codereview.chromium.org/2702463003/diff/80001/base/BUILD.gn > File base/BUILD.gn (right): > > https://codereview.chromium.org/2702463003/diff/80001/base/BUILD.gn#newcode1828 > base/BUILD.gn:1828: shared_library("base_profiler_test_support_library") { ...
4 months ago (2017-02-17 18:10:20 UTC) #67
Avi (ping after 24h)
On 2017/02/17 18:10:20, Mark Mentovai wrote: > Avi wrote: > > https://codereview.chromium.org/2702463003/diff/80001/base/BUILD.gn > > File ...
4 months ago (2017-02-17 18:19:56 UTC) #68
Avi (ping after 24h)
On 2017/02/17 18:19:56, Avi wrote: > On 2017/02/17 18:10:20, Mark Mentovai wrote: > > Avi ...
4 months ago (2017-02-17 21:20:39 UTC) #71
Avi (ping after 24h)
Here is a sample run with logging in ReceiveCompletedProfilesImpl: ReceiveCompletedProfilesImpl > received 1 profiles >> ...
4 months ago (2017-02-17 21:33:41 UTC) #72
Mike Wittman
On 2017/02/17 21:33:41, Avi wrote: > Here is a sample run with logging in ReceiveCompletedProfilesImpl: ...
4 months ago (2017-02-17 21:57:42 UTC) #73
Avi (ping after 24h)
On 2017/02/17 21:57:42, Mike Wittman wrote: > On 2017/02/17 21:33:41, Avi wrote: > > Here ...
4 months ago (2017-02-17 22:04:12 UTC) #74
Mike Wittman
On 2017/02/17 22:04:12, Avi wrote: > On 2017/02/17 21:57:42, Mike Wittman wrote: > > On ...
4 months ago (2017-02-17 22:18:27 UTC) #75
Avi (ping after 24h)
I'm not even to the 9 frame stacks. We're stuck with two issues. The first ...
4 months ago (2017-02-17 22:46:57 UTC) #76
Mark Mentovai
On 2017/02/17 22:46:57, Avi wrote: > I'm not even to the 9 frame stacks. > ...
4 months ago (2017-02-19 16:23:27 UTC) #77
Avi (ping after 24h)
You are correct; this is due to an error during hacking to figure out what's ...
4 months ago (2017-02-19 19:05:00 UTC) #78
Avi (ping after 24h)
On 2017/02/19 19:05:00, Avi wrote: > I'm going to dig a bit with that approach. ...
4 months ago (2017-02-19 19:46:02 UTC) #79
Avi (ping after 24h)
On 2017/02/19 19:46:02, Avi wrote: > On 2017/02/19 19:05:00, Avi wrote: > > I'm going ...
4 months ago (2017-02-19 21:19:30 UTC) #80
Avi (ping after 24h)
As a reply to two posts ago, I modified the stack unwinder to keep at ...
4 months ago (2017-02-20 00:01:11 UTC) #81
Mark Mentovai
First, lemme capture my previous e-mail to you here to give context to the review ...
4 months ago (2017-02-20 00:06:28 UTC) #82
Mark Mentovai
The trick that I gave you previously was “rip = *rsp; rsp += 8;”. That’s ...
4 months ago (2017-02-20 00:32:10 UTC) #83
Avi (ping after 24h)
There are all kinds of leaf functions, as I ran into last time: +[NSObject alloc] ...
4 months ago (2017-02-20 00:39:47 UTC) #84
Mark Mentovai
Avi wrote: > There are all kinds of leaf functions, as I ran into last ...
4 months ago (2017-02-20 05:23:05 UTC) #85
Mike Wittman
Providing a little context here: it's OK from the analysis perspective if we can't recover ...
3 months, 3 weeks ago (2017-02-24 16:01:13 UTC) #86
Avi (ping after 24h)
On 2017/02/24 16:01:13, Mike Wittman wrote: > Providing a little context here: it's OK from ...
3 months, 3 weeks ago (2017-02-24 18:43:03 UTC) #87
Avi (ping after 24h)
On 2017/02/20 05:23:05, Mark Mentovai wrote: > By now you know how to get these: ...
3 months, 3 weeks ago (2017-02-24 18:49:42 UTC) #88
Mike Wittman
On 2017/02/24 18:43:03, Avi wrote: > On 2017/02/24 16:01:13, Mike Wittman wrote: > > Providing ...
3 months, 3 weeks ago (2017-02-24 18:51:24 UTC) #89
Avi (ping after 24h)
Mike, Mark, Robert, please review. BTW, does anyone know what's up the with ios simulator ...
3 months, 3 weeks ago (2017-03-01 20:11:37 UTC) #102
Mike Wittman
On 2017/03/01 20:11:37, Avi wrote: > Mike, Mark, Robert, please review. How do the resulting ...
3 months, 3 weeks ago (2017-03-02 16:35:04 UTC) #103
Avi (ping after 24h)
https://codereview.chromium.org/2702463003/diff/200001/base/profiler/native_stack_sampler_mac.cc File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2702463003/diff/200001/base/profiler/native_stack_sampler_mac.cc#newcode175 base/profiler/native_stack_sampler_mac.cc:175: do { On 2017/03/02 16:35:04, Mike Wittman wrote: > ...
3 months, 3 weeks ago (2017-03-02 19:50:44 UTC) #106
Avi (ping after 24h)
On 2017/03/02 16:35:04, Mike Wittman wrote: > How do the resulting stacks look in local ...
3 months, 3 weeks ago (2017-03-02 20:04:00 UTC) #107
Mike Wittman
OK, I'm generally satisfied with this as an initial implementation if Mark and Robert are ...
3 months, 2 weeks ago (2017-03-03 20:21:58 UTC) #110
Mike Wittman
https://codereview.chromium.org/2702463003/diff/220001/base/profiler/native_stack_sampler_mac.cc File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2702463003/diff/220001/base/profiler/native_stack_sampler_mac.cc#newcode125 base/profiler/native_stack_sampler_mac.cc:125: callback(static_cast<uintptr_t>(ip)); This and a few other places are using ...
3 months, 2 weeks ago (2017-03-03 20:22:23 UTC) #111
Avi (ping after 24h)
I'm in agreement. I don't like the 1% figure, but I'd like to run with ...
3 months, 2 weeks ago (2017-03-03 20:36:47 UTC) #114
Avi (ping after 24h)
iOS issue fixed. Mark, Robert, thoughts?
3 months, 2 weeks ago (2017-03-06 20:34:52 UTC) #119
Avi (ping after 24h)
On 2017/03/06 20:34:52, Avi wrote: > iOS issue fixed. > > Mark, Robert, thoughts? Oh, ...
3 months, 2 weeks ago (2017-03-07 00:15:39 UTC) #122
Mike Wittman
On 2017/03/07 00:15:39, Avi wrote: > On 2017/03/06 20:34:52, Avi wrote: > > iOS issue ...
3 months, 2 weeks ago (2017-03-08 18:09:20 UTC) #123
Robert Sesek
Overall LG, just some minor comments. https://codereview.chromium.org/2702463003/diff/260001/base/profiler/native_stack_sampler_mac.cc File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2702463003/diff/260001/base/profiler/native_stack_sampler_mac.cc#newcode54 base/profiler/native_stack_sampler_mac.cc:54: uintptr_t original_stack_bottom_int = ...
3 months, 1 week ago (2017-03-15 21:57:26 UTC) #124
Mike Wittman
https://codereview.chromium.org/2702463003/diff/260001/base/profiler/native_stack_sampler_mac.cc File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2702463003/diff/260001/base/profiler/native_stack_sampler_mac.cc#newcode413 base/profiler/native_stack_sampler_mac.cc:413: if (stack_size > kStackCopyBufferSize) On 2017/03/15 21:57:25, Robert Sesek ...
3 months, 1 week ago (2017-03-15 22:09:56 UTC) #125
Mark Mentovai
https://codereview.chromium.org/2702463003/diff/260001/base/profiler/native_stack_sampler_mac.cc File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2702463003/diff/260001/base/profiler/native_stack_sampler_mac.cc#newcode54 base/profiler/native_stack_sampler_mac.cc:54: uintptr_t original_stack_bottom_int = I see these casts here, there’s ...
3 months, 1 week ago (2017-03-16 03:08:46 UTC) #126
Mike Wittman
https://codereview.chromium.org/2702463003/diff/260001/base/profiler/native_stack_sampler_mac.cc File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2702463003/diff/260001/base/profiler/native_stack_sampler_mac.cc#newcode122 base/profiler/native_stack_sampler_mac.cc:122: ++(*frame_count); On 2017/03/16 03:08:46, Mark Mentovai wrote: > It ...
3 months, 1 week ago (2017-03-16 16:42:24 UTC) #127
Mark Mentovai
https://codereview.chromium.org/2702463003/diff/260001/base/profiler/native_stack_sampler_mac.cc File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2702463003/diff/260001/base/profiler/native_stack_sampler_mac.cc#newcode183 base/profiler/native_stack_sampler_mac.cc:183: rsp += 8; // rsp++ One more thought for ...
3 months, 1 week ago (2017-03-16 16:53:46 UTC) #128
Avi (ping after 24h)
https://codereview.chromium.org/2702463003/diff/260001/base/profiler/native_stack_sampler_mac.cc File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2702463003/diff/260001/base/profiler/native_stack_sampler_mac.cc#newcode54 base/profiler/native_stack_sampler_mac.cc:54: uintptr_t original_stack_bottom_int = On 2017/03/16 03:08:46, Mark Mentovai wrote: ...
3 months ago (2017-03-18 03:09:28 UTC) #141
Mike Wittman
https://codereview.chromium.org/2702463003/diff/260001/base/profiler/native_stack_sampler_mac.cc File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2702463003/diff/260001/base/profiler/native_stack_sampler_mac.cc#newcode54 base/profiler/native_stack_sampler_mac.cc:54: uintptr_t original_stack_bottom_int = On 2017/03/18 03:09:27, Avi wrote: > ...
3 months ago (2017-03-20 19:21:31 UTC) #142
Avi (ping after 24h)
Mark, Robert: I addressed your comments a week ago. Ping. Please review. https://codereview.chromium.org/2702463003/diff/320001/chrome/common/stack_sampling_configuration.cc File chrome/common/stack_sampling_configuration.cc ...
2 months, 3 weeks ago (2017-03-27 16:06:30 UTC) #143
Mark Mentovai
LGTM https://codereview.chromium.org/2702463003/diff/260001/base/profiler/native_stack_sampler_mac.cc File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2702463003/diff/260001/base/profiler/native_stack_sampler_mac.cc#newcode122 base/profiler/native_stack_sampler_mac.cc:122: ++(*frame_count); On 2017/03/18 03:09:27, Avi wrote: > On ...
2 months, 3 weeks ago (2017-03-27 18:24:55 UTC) #148
Mike Wittman
https://codereview.chromium.org/2702463003/diff/260001/base/profiler/native_stack_sampler_mac.cc File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2702463003/diff/260001/base/profiler/native_stack_sampler_mac.cc#newcode54 base/profiler/native_stack_sampler_mac.cc:54: uintptr_t original_stack_bottom_int = On 2017/03/20 19:21:30, Mike Wittman wrote: ...
2 months, 3 weeks ago (2017-03-27 19:48:47 UTC) #149
Avi (ping after 24h)
Nico, the config file? https://codereview.chromium.org/2702463003/diff/260001/base/profiler/native_stack_sampler_mac.cc File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2702463003/diff/260001/base/profiler/native_stack_sampler_mac.cc#newcode54 base/profiler/native_stack_sampler_mac.cc:54: uintptr_t original_stack_bottom_int = On 2017/03/27 ...
2 months, 3 weeks ago (2017-03-29 17:52:09 UTC) #152
Mike Wittman
https://codereview.chromium.org/2702463003/diff/340001/base/profiler/native_stack_sampler_mac.cc File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2702463003/diff/340001/base/profiler/native_stack_sampler_mac.cc#newcode308 base/profiler/native_stack_sampler_mac.cc:308: static constexpr size_t kStackCopyBufferSize = 12 * 1024 * ...
2 months, 3 weeks ago (2017-03-29 18:08:09 UTC) #154
Avi (ping after 24h)
https://codereview.chromium.org/2702463003/diff/340001/base/profiler/native_stack_sampler_mac.cc File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2702463003/diff/340001/base/profiler/native_stack_sampler_mac.cc#newcode308 base/profiler/native_stack_sampler_mac.cc:308: static constexpr size_t kStackCopyBufferSize = 12 * 1024 * ...
2 months, 3 weeks ago (2017-03-29 19:04:15 UTC) #155
Nico
config file lgtm
2 months, 3 weeks ago (2017-03-29 19:19:36 UTC) #156
Mark Mentovai
https://codereview.chromium.org/2702463003/diff/340001/base/profiler/native_stack_sampler_mac.cc File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2702463003/diff/340001/base/profiler/native_stack_sampler_mac.cc#newcode308 base/profiler/native_stack_sampler_mac.cc:308: static constexpr size_t kStackCopyBufferSize = 12 * 1024 * ...
2 months, 3 weeks ago (2017-03-29 19:23:50 UTC) #157
Avi (ping after 24h)
https://codereview.chromium.org/2702463003/diff/340001/base/profiler/native_stack_sampler_mac.cc File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2702463003/diff/340001/base/profiler/native_stack_sampler_mac.cc#newcode308 base/profiler/native_stack_sampler_mac.cc:308: static constexpr size_t kStackCopyBufferSize = 12 * 1024 * ...
2 months, 3 weeks ago (2017-03-29 19:59:25 UTC) #158
Mike Wittman
https://codereview.chromium.org/2702463003/diff/340001/base/profiler/native_stack_sampler_mac.cc File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2702463003/diff/340001/base/profiler/native_stack_sampler_mac.cc#newcode308 base/profiler/native_stack_sampler_mac.cc:308: static constexpr size_t kStackCopyBufferSize = 12 * 1024 * ...
2 months, 3 weeks ago (2017-03-29 20:03:02 UTC) #159
Avi (ping after 24h)
Mark, Mike, PTAL
2 months, 3 weeks ago (2017-03-30 00:46:18 UTC) #164
Mark Mentovai
LGTM otherwise https://codereview.chromium.org/2702463003/diff/380001/base/profiler/native_stack_sampler_mac.cc File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2702463003/diff/380001/base/profiler/native_stack_sampler_mac.cc#newcode49 base/profiler/native_stack_sampler_mac.cc:49: // of 8 MB with extra wiggle ...
2 months, 3 weeks ago (2017-03-30 02:33:27 UTC) #167
Mike Wittman
LGTM Can you retest this and validate that the recovered stack percentage is still in ...
2 months, 3 weeks ago (2017-03-30 16:56:46 UTC) #168
Avi (ping after 24h)
Nothing changes re completeness. I just ran a sampling. The GPU process is in better ...
2 months, 3 weeks ago (2017-03-30 20:13:38 UTC) #171
Mark Mentovai
LGTM https://codereview.chromium.org/2702463003/diff/380001/base/profiler/native_stack_sampler_mac.cc File base/profiler/native_stack_sampler_mac.cc (right): https://codereview.chromium.org/2702463003/diff/380001/base/profiler/native_stack_sampler_mac.cc#newcode49 base/profiler/native_stack_sampler_mac.cc:49: // of 8 MB with extra wiggle room. ...
2 months, 3 weeks ago (2017-03-30 20:18:15 UTC) #172
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2702463003/400001
2 months, 3 weeks ago (2017-03-31 15:57:48 UTC) #177
commit-bot: I haz the power
Committed patchset #21 (id:400001) as https://chromium.googlesource.com/chromium/src/+/965a47b7737c6f15238447d8d8fb7105183acc8a
2 months, 3 weeks ago (2017-03-31 16:40:29 UTC) #180
Avi (ping after 24h)
2 months, 2 weeks ago (2017-04-04 16:06:09 UTC) #181
Message was sent while issue was closed.
A revert of this CL (patchset #21 id:400001) has been created in
https://codereview.chromium.org/2797513003/ by avi@chromium.org.

The reason for reverting is: WebGL crashes. http://crbug.com/707474.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318