|
|
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. |
DescriptionStatistical 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 #
Messages
Total messages: 24 (6 generated)
wittman@chromium.org changed reviewers: + cpu@chromium.org
PTAL This CL is split off from the metrics provider in https://crrev.com/981143006, and I've added tests for the profiler.
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:20: namespace { Add a blank line after this. https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:24: public: Indent is wrong. "git cl format"? https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:126: Profile* profile, TimeDelta* elapsed_time) { 1 param per line if first one wraps. https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:132: for (int j = 0; j < params_.samples_per_burst; ++j) { Why j? https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler_posix.cc (right): https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_posix.cc:10: StackSamplingProfiler::NativeStackSampler::Create( I think for cases like these, convention is not to indent here. Perhaps just run "git cl format"? (Or did it produce this indentation?) https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_posix.cc:15: } Nit: // base https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:23: NativeStackSamplerWin(win::ScopedHandle thread_handle); explicit https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:26: void ProfileRecordingStarting( Add a comment: // StackSamplingProfiler::NativeStackSampler: before this block https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:41: StackSamplingProfiler::Profile* current_profile_; Please add comments. What's the ownership of this? https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:42: std::map<HMODULE, int> profile_module_index_; Please add comments, what's this mapping to? https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:143: } Nit: // namespace base https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:227: const void* const instruction_pointers[], const HMODULE modules[], Nit: 1 param per line, when first param is on a separate line in decl. Maybe run "git cl format"? (assuming it works on Windows) https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:235: StackSamplingProfiler::Frame& frame = sample->back(); Non-const refs are discouraged. Use a pointer. Or, put it on the stack and push back at the end of the function. https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:237: if (!modules[i]) Is it correct to have an entry in |sample| in this case?
https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:20: namespace { On 2015/03/17 13:10:00, Alexei Svitkine (away) wrote: > Add a blank line after this. Done. https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:24: public: On 2015/03/17 13:10:00, Alexei Svitkine (away) wrote: > Indent is wrong. "git cl format"? Done. https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:126: Profile* profile, TimeDelta* elapsed_time) { On 2015/03/17 13:10:00, Alexei Svitkine (away) wrote: > 1 param per line if first one wraps. Done. https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:132: for (int j = 0; j < params_.samples_per_burst; ++j) { On 2015/03/17 13:10:00, Alexei Svitkine (away) wrote: > Why j? This is an artifact from when this code was part of CollectProfiles. Changed to i. https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler_posix.cc (right): https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_posix.cc:10: StackSamplingProfiler::NativeStackSampler::Create( On 2015/03/17 13:10:00, Alexei Svitkine (away) wrote: > I think for cases like these, convention is not to indent here. Perhaps just run > "git cl format"? (Or did it produce this indentation?) Yes, that's correct. Unindented. https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_posix.cc:15: } On 2015/03/17 13:10:00, Alexei Svitkine (away) wrote: > Nit: // base Done. https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:23: NativeStackSamplerWin(win::ScopedHandle thread_handle); On 2015/03/17 13:10:00, Alexei Svitkine (away) wrote: > explicit Done. https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:26: void ProfileRecordingStarting( On 2015/03/17 13:10:00, Alexei Svitkine (away) wrote: > Add a comment: > > // StackSamplingProfiler::NativeStackSampler: > > before this block Done. https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:41: StackSamplingProfiler::Profile* current_profile_; On 2015/03/17 13:10:00, Alexei Svitkine (away) wrote: > Please add comments. What's the ownership of this? Done. https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:42: std::map<HMODULE, int> profile_module_index_; On 2015/03/17 13:10:00, Alexei Svitkine (away) wrote: > Please add comments, what's this mapping to? Done. https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:143: } On 2015/03/17 13:10:00, Alexei Svitkine (away) wrote: > Nit: // namespace base Done. (This is end of the anonymous namespace.) https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:227: const void* const instruction_pointers[], const HMODULE modules[], On 2015/03/17 13:10:00, Alexei Svitkine (away) wrote: > Nit: 1 param per line, when first param is on a separate line in decl. Maybe run > "git cl format"? (assuming it works on Windows) Done. https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:235: StackSamplingProfiler::Frame& frame = sample->back(); On 2015/03/17 13:10:00, Alexei Svitkine (away) wrote: > Non-const refs are discouraged. Use a pointer. Or, put it on the stack and push > back at the end of the function. They are for function parameters, but aliases are a different matter. The Google C++ Style Guide explicitly recommends this type of usage: "Instead of using a macro to "abbreviate" a long variable name, use a reference." It also appears to be not uncommon in the Chrome codebase. https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:237: if (!modules[i]) On 2015/03/17 13:10:00, Alexei Svitkine (away) wrote: > Is it correct to have an entry in |sample| in this case? Yes. In this case, we saw a frame on the stack but don't know what module it was from. An empty Frame is the best representation of this state. Frame's contents weren't being initialized though, so I added a default constructor. Also added a clarifying comment here.
https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1016563004/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler_win.cc:237: if (!modules[i]) On 2015/03/17 19:07:38, Mike Wittman wrote: > On 2015/03/17 13:10:00, Alexei Svitkine (away) wrote: > > Is it correct to have an entry in |sample| in this case? > > Yes. In this case, we saw a frame on the stack but don't know what module it was > from. An empty Frame is the best representation of this state. > > Frame's contents weren't being initialized though, so I added a default > constructor. Also added a clarifying comment here. On second thought, storing the instruction pointer is still useful in this case. It's only at the UMA level that we need to provide an empty frame since we can't compute a valid instruction pointer offset.
zturner@chromium.org changed reviewers: + zturner@chromium.org
Mostly commenting on the Windows stuff. I'll leave the posix stuff for others. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:66: // GUID + AGE in the debug image headers of a module. Is the GUID + AGE always available even for for release / optimized builds? Why not just a SHA1 of the binary, which would be the same for all platforms. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:85: using Sample = std::vector<Frame>; I'm not sure what the Chromium's build requirements are these days, but are template aliases ok? I thought they miscompiled on versions of MSVC prior to 2015. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:111: static scoped_ptr<NativeStackSampler> Create(PlatformThreadId thread_id); It's a little odd to see a scoped_ptr returned by value. Should this be unique_ptr? https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:115: virtual void ProfileRecordingStarting(Profile* profile) = 0; Is there any reason to expect the user might pass nullptr? It might makes the function kind of useless if so, and a reference seems better if not. Similar comment applies to other subsequent functions in this file. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:77: RtlVirtualUnwind(0, image_base, context->Rip, runtime_function, context, Did you consider using RtlCaptureStackBacktrace? It's a function which is implemented on top of RtlVirtualUnwind, with its only limitation being that it does not return the non-volatile register contexts for each frame. You don't appear to use any of this data, so you could simplify this code quite a bit by just using RtlCaptureStackBackTrace. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:92: return 0; Is there any desire to support x86? RtlVirtualUnwind isn't supported on x86, but StackWalk64 is, and you can use StackWalk64 to walk yourself. You need to use RtlCaptureContext() to get a CONTEXT for yourself, then fill out the fields of a STACKFRAME64 with the values from the CONTEXT and call it with GetCurrentProcess() and GetCurrentThread(). I'm not sure how well it works without symbols, but it might be worth an experiment? Unless you don't care about x86 by design, in which case ignore this. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:105: GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT, Why not increment the module's refcount? You store the handle, so it seems risky to not increment the refcount. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:113: for (int i = module_frames; i < stack_depth; ++i) Alternatively, you could memset() the entire array to 0 upon startup. Seems a little clearer what the intent is in that case. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:114: modules[stack_depth - 1] = NULL; What about addresses? You're going to end up with junk in the remainder of the addresses[] array. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:123: if (::SuspendThread(thread_handle) == -1) { Just to be certain, this will never be equal to the current thread right? https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:129: thread_context.ContextFlags = CONTEXT_ALL; Do you actually need CONTEXT_ALL? CONTEXT_FULL is (oddly) more lightweight. CONTEXT_ALL will get debug registers, floating point registers, and extended registers like the MMX registers. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:154: HANDLE thread_handle = ::OpenThread(THREAD_ALL_ACCESS, FALSE, thread_id); THREAD_GET_CONTEXT | THREAD_SUSPEND_RESUME | THREAD_QUERY_INFORMATION should be all you need. I'm not sure which is less likely to fail in practice when you're talking about a billion users. But I feel like asking for only as much privilege as you need is better, even if we do control the thread. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:168: const HMODULE nt_dll_handle = ::GetModuleHandle(L"ntdll.dll"); You should probably handle the case where nt_dll_handle == nullptr. It shouldn't happen in practice, but still should handle it. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:192: HMODULE modules[max_stack_size]; If you initialize these two arrays with "= {0}" then you can skip the loop in the SuspendThreadAndRecordStack function and also fix the issue I pointed out with that function earlier where instruction_pointers[] ends up with junk. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:220: win::PEImage(module).GetDebugId(&guid, &age); Is this present for release / optimized builds?
danduong@chromium.org changed reviewers: + danduong@chromium.org
https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:66: // GUID + AGE in the debug image headers of a module. On 2015/03/18 23:04:37, zturner wrote: > Is the GUID + AGE always available even for for release / optimized builds? Why > not just a SHA1 of the binary, which would be the same for all platforms. This needs to be the same Module ID that breakpad uses for identifying a symbol. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:77: RtlVirtualUnwind(0, image_base, context->Rip, runtime_function, context, On 2015/03/18 23:04:37, zturner wrote: > Did you consider using RtlCaptureStackBacktrace? It's a function which is > implemented on top of RtlVirtualUnwind, with its only limitation being that it > does not return the non-volatile register contexts for each frame. You don't > appear to use any of this data, so you could simplify this code quite a bit by > just using RtlCaptureStackBackTrace. As far as I know, RtlCaptureStackBackTrace cannot grab a stacktrace of another thread. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:92: return 0; On 2015/03/18 23:04:37, zturner wrote: > Is there any desire to support x86? RtlVirtualUnwind isn't supported on x86, > but StackWalk64 is, and you can use StackWalk64 to walk yourself. You need to > use RtlCaptureContext() to get a CONTEXT for yourself, then fill out the fields > of a STACKFRAME64 with the values from the CONTEXT and call it with > GetCurrentProcess() and GetCurrentThread(). I'm not sure how well it works > without symbols, but it might be worth an experiment? Unless you don't care > about x86 by design, in which case ignore this. We'll focus on x64 for now. StackWalk64, I've heard, can deadlock if you're using it after SuspendThread. It also tries to resolve symbols which is unnecessary overhead. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:220: win::PEImage(module).GetDebugId(&guid, &age); On 2015/03/18 23:04:37, zturner wrote: > Is this present for release / optimized builds? Should be. Minidumps include the debugIds of the modules.
https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:85: using Sample = std::vector<Frame>; On 2015/03/18 23:04:36, zturner wrote: > I'm not sure what the Chromium's build requirements are these days, but are > template aliases ok? I thought they miscompiled on versions of MSVC prior to > 2015. Guideline is to use "using" instead of typedef. We've only seen breakage when used with template template functions: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/8dOAMzgR4ao/mOR2G... https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:111: static scoped_ptr<NativeStackSampler> Create(PlatformThreadId thread_id); On 2015/03/18 23:04:36, zturner wrote: > It's a little odd to see a scoped_ptr returned by value. Should this be > unique_ptr? We don't have unique_ptr in Chrome yet. scoped_ptr is the standard for ownership transfer. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:115: virtual void ProfileRecordingStarting(Profile* profile) = 0; On 2015/03/18 23:04:36, zturner wrote: > Is there any reason to expect the user might pass nullptr? It might makes the > function kind of useless if so, and a reference seems better if not. Similar > comment applies to other subsequent functions in this file. Only const objects may be passed by reference according to the C++ style guide. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:105: GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT, On 2015/03/18 23:04:37, zturner wrote: > Why not increment the module's refcount? You store the handle, so it seems > risky to not increment the refcount. Incrementing the refcount led to deadlock previously when this was called between SuspendThread/ResumeThread. Now that it's outside of those calls it appears to work properly. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:113: for (int i = module_frames; i < stack_depth; ++i) On 2015/03/18 23:04:37, zturner wrote: > Alternatively, you could memset() the entire array to 0 upon startup. Seems a > little clearer what the intent is in that case. Used a zero initializer. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:114: modules[stack_depth - 1] = NULL; On 2015/03/18 23:04:37, zturner wrote: > What about addresses? You're going to end up with junk in the remainder of the > addresses[] array. Done. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:123: if (::SuspendThread(thread_handle) == -1) { On 2015/03/18 23:04:37, zturner wrote: > Just to be certain, this will never be equal to the current thread right? Yes, this is only ever called on the profiler thread. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:129: thread_context.ContextFlags = CONTEXT_ALL; On 2015/03/18 23:04:37, zturner wrote: > Do you actually need CONTEXT_ALL? CONTEXT_FULL is (oddly) more lightweight. > CONTEXT_ALL will get debug registers, floating point registers, and extended > registers like the MMX registers. CONTEXT_FULL appears to be sufficient. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:154: HANDLE thread_handle = ::OpenThread(THREAD_ALL_ACCESS, FALSE, thread_id); On 2015/03/18 23:04:37, zturner wrote: > THREAD_GET_CONTEXT | THREAD_SUSPEND_RESUME | THREAD_QUERY_INFORMATION should be > all you need. I'm not sure which is less likely to fail in practice when you're > talking about a billion users. But I feel like asking for only as much > privilege as you need is better, even if we do control the thread. Done. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:168: const HMODULE nt_dll_handle = ::GetModuleHandle(L"ntdll.dll"); On 2015/03/18 23:04:37, zturner wrote: > You should probably handle the case where nt_dll_handle == nullptr. It > shouldn't happen in practice, but still should handle it. Done. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:192: HMODULE modules[max_stack_size]; On 2015/03/18 23:04:37, zturner wrote: > If you initialize these two arrays with "= {0}" then you can skip the loop in > the SuspendThreadAndRecordStack function and also fix the issue I pointed out > with that function earlier where instruction_pointers[] ends up with junk. Done.
https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.cc:195: delete thread; why do we need a special deleter? https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.cc:203: : initial_delay(TimeDelta::FromMilliseconds(0)), bursts(1), bursts() in next line https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:69: FilePath filename; are you getting the full path or just the name? https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:176: Callback<void(const std::vector<Profile>&)> callback); feels strange to get them all ... why not the last one only?
https://codereview.chromium.org/1016563004/diff/60001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1016563004/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:148: if (::ResumeThread(thread_handle) == -1) One minor comment here. This will cause the thread to get a priority boost. If you want to minimize the impact that the sampler has on the program's normal behavior, you may want to consider using SetThreadPriorityBoost() before calling this function to disable the priority boost, and then call it again after ResumeThread() returns to restore the normal state. Up to you to decide whether this is important, mostly just mentioning it here since it's easy to overlook.
https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:69: FilePath filename; On 2015/03/19 02:49:04, cpu wrote: > are you getting the full path or just the name? This should be full path. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:176: Callback<void(const std::vector<Profile>&)> callback); On 2015/03/19 02:49:04, cpu wrote: > feels strange to get them all ... why not the last one only? Are you suggesting we should call the callback on completion of each burst? That might be reasonable. What do you think Mike?
https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.cc:195: delete thread; On 2015/03/19 02:49:04, cpu wrote: > why do we need a special deleter? To keep the definition of SamplingThread out of the header. scoped_ptr needs a complete type unless we provide a custom deleter. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.cc:203: : initial_delay(TimeDelta::FromMilliseconds(0)), bursts(1), On 2015/03/19 02:49:04, cpu wrote: > bursts() in next line Done. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:69: FilePath filename; On 2015/03/19 03:09:45, danduong wrote: > On 2015/03/19 02:49:04, cpu wrote: > > are you getting the full path or just the name? > > This should be full path. Yes, this is the full path. When sending to UMA we only consider the basename. https://codereview.chromium.org/1016563004/diff/40001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:176: Callback<void(const std::vector<Profile>&)> callback); On 2015/03/19 03:09:45, danduong wrote: > On 2015/03/19 02:49:04, cpu wrote: > > feels strange to get them all ... why not the last one only? > > Are you suggesting we should call the callback on completion of each burst? That > might be reasonable. What do you think Mike? For providing data to UMA I don't think it matters much either way (although that pathway doesn't use a custom callback anyway). For ad hoc use within Chrome, returning all profiles has the nice property that once you get the callback you know the profiling is complete. This allows threads to profile themselves and process the results, without executing profile-processing code while being profiled. https://codereview.chromium.org/1016563004/diff/60001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler_win.cc (right): https://codereview.chromium.org/1016563004/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_win.cc:148: if (::ResumeThread(thread_handle) == -1) On 2015/03/19 02:54:12, zturner wrote: > One minor comment here. This will cause the thread to get a priority boost. If > you want to minimize the impact that the sampler has on the program's normal > behavior, you may want to consider using SetThreadPriorityBoost() before calling > this function to disable the priority boost, and then call it again after > ResumeThread() returns to restore the normal state. Up to you to decide whether > this is important, mostly just mentioning it here since it's easy to overlook. Thanks for the heads up. I don't think we want priority boost, particularly for non-UI threads. Added a scoped disable.
lgtm for the windows part, we need a base/ reviewer that understands the posix side. Or you can land the windows part. Thanks zturner for the nice review, anything else of concern? https://codereview.chromium.org/1016563004/diff/80001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler_unittest.cc (right): https://codereview.chromium.org/1016563004/diff/80001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_unittest.cc:211: ASSERT_TRUE(loc != sample.end()) << "function not found in stack"; in the failure case, lets dump what we got to stdout so the people looking at the bots (sheriffs) get a chance to paste meaninful info on a bug.
On 2015/03/20 19:34:41, cpu wrote: > lgtm for the windows part, we need a base/ reviewer that understands the posix > side. > > Or you can land the windows part. > > Thanks zturner for the nice review, anything else of concern? > > https://codereview.chromium.org/1016563004/diff/80001/base/profiler/stack_sam... > File base/profiler/stack_sampling_profiler_unittest.cc (right): > > https://codereview.chromium.org/1016563004/diff/80001/base/profiler/stack_sam... > base/profiler/stack_sampling_profiler_unittest.cc:211: ASSERT_TRUE(loc != > sample.end()) << "function not found in stack"; > in the failure case, lets dump what we got to stdout so the people looking at > the bots (sheriffs) get a chance to paste meaninful info on a bug. No, lgtm
On 2015/03/20 19:38:14, zturner wrote: > On 2015/03/20 19:34:41, cpu wrote: > > lgtm for the windows part, we need a base/ reviewer that understands the posix > > side. > > > > Or you can land the windows part. > > > > Thanks zturner for the nice review, anything else of concern? > > > No, lgtm Thanks! I'll land the windows part now, and address posix once we have a non-empty implementation there.
https://codereview.chromium.org/1016563004/diff/80001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler_unittest.cc (right): https://codereview.chromium.org/1016563004/diff/80001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler_unittest.cc:211: ASSERT_TRUE(loc != sample.end()) << "function not found in stack"; On 2015/03/20 19:34:41, cpu wrote: > in the failure case, lets dump what we got to stdout so the people looking at > the bots (sheriffs) get a chance to paste meaninful info on a bug. Done.
The CQ bit was checked by wittman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org, zturner@chromium.org Link to the patchset: https://codereview.chromium.org/1016563004/#ps100001 (title: "add diagnostic output for test failure")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1016563004/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b450e197e878aec522e7299004dba511c65f254c Cr-Commit-Position: refs/heads/master@{#321658} |