Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
888923004:
Temporary commit to evaluate perf impact of prototype CPU profiler (Closed)
|
Created:
5 years, 10 months ago by Mike Wittman Modified:
5 years, 10 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, Ilya Sherman Base URL:
https://chromium.googlesource.com/chromium/src.git@lkcr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTemporary commit to evaluate perf impact of prototype CPU profiler
Design doc: https://docs.google.com/document/d/1Bv-I1yM9AjgY3t27ixdyWHCAW9IpVx02YQ-nSbIqccc
BUG=
Committed: https://crrev.com/47c6093645b62fbc9f68cf50375fc0626e32516c
Cr-Commit-Position: refs/heads/master@{#315196}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #Patch Set 5 : . #
Total comments: 30
Patch Set 6 : remove switch #
Total comments: 13
Patch Set 7 : address comments #
Total comments: 12
Patch Set 8 : address comments #
Total comments: 17
Patch Set 9 : address comments #
Total comments: 2
Patch Set 10 : address comments #
Messages
Total messages: 34 (9 generated)
wittman@chromium.org changed reviewers: + cpu@chromium.org
Hi Carlos, this is an initial Win x64 implementation of the stack sampling for the statistical profiler than danduong@ is proposing. I'd like to run this through one pass on the perf bots, to quantify the overhead of this technique. Can I get an LGTM to commit, followed by a revert once we have perf results? Any comments on the implementation of the technique also would be welcome. I'm aware that the code is not up to Chrome standards at this point, so no need to comment in that area unless you feel it should block the temporary commit. (I do plan to remove the unused switch before committing.)
danduong@chromium.org changed reviewers: + wfh@chromium.org
cpu@chromium.org changed reviewers: + isherman@chromium.org
Adding isherman since he has made or touched most of base/profiler
On 2015/01/31 02:18:37, Mike Wittman wrote: > Hi Carlos, this is an initial Win x64 implementation of the stack sampling for > the statistical profiler than danduong@ is proposing. > > I'd like to run this through one pass on the perf bots, to quantify the overhead > of this technique. Can I get an LGTM to commit, followed by a revert once we > have perf results? > > Any comments on the implementation of the technique also would be welcome. > > I'm aware that the code is not up to Chrome standards at this point, so no need > to comment in that area unless you feel it should block the temporary commit. (I > do plan to remove the unused switch before committing.) hi - is there a design doc for the "statistical profiler than danduong@ is proposing."?
please do a standalone CL with stack_trace.h and .cc I think we can take those two as a permantent change, so we won't be reverting those two files.
On 2015/02/03 01:23:11, Will Harris wrote: > On 2015/01/31 02:18:37, Mike Wittman wrote: > > Hi Carlos, this is an initial Win x64 implementation of the stack sampling for > > the statistical profiler than danduong@ is proposing. > > > > I'd like to run this through one pass on the perf bots, to quantify the > overhead > > of this technique. Can I get an LGTM to commit, followed by a revert once we > > have perf results? > > > > Any comments on the implementation of the technique also would be welcome. > > > > I'm aware that the code is not up to Chrome standards at this point, so no > need > > to comment in that area unless you feel it should block the temporary commit. > (I > > do plan to remove the unused switch before committing.) > > hi - is there a design doc for the "statistical profiler than danduong@ is > proposing."? Yes: https://docs.google.com/document/d/1Bv-I1yM9AjgY3t27ixdyWHCAW9IpVx02YQ-nSbIqccc
isherman@chromium.org changed reviewers: + vadimt@chromium.org
Adding Vadim, since he is currently far more involved with profiler work than I am. At a meta-level, I'm curious as to the expected value-add over the existing profilers. Is it perhaps worth focusing our efforts on squeezing out information from the data that we have, rather than splitting the profiling work across multiple profiling approaches? Or can we move faster by implementing these two parallel approaches?
wittman@chromium.org changed reviewers: + danduong@chromium.org
wittman@chromium.org changed reviewers: + vadimt@google.com - danduong@chromium.org, vadimt@chromium.org
On 2015/02/03 01:44:36, Ilya Sherman wrote: > Adding Vadim, since he is currently far more involved with profiler work than I > am. At a meta-level, I'm curious as to the expected value-add over the existing > profilers. Is it perhaps worth focusing our efforts on squeezing out > information from the data that we have, rather than splitting the profiling work > across multiple profiling approaches? Or can we move faster by implementing > these two parallel approaches? +danduong, who is probably the best person to address the meta-level questions.
On 2015/02/03 01:31:32, cpu wrote: > please do a standalone CL with stack_trace.h and .cc > I think we can take those two as a permantent change, so we won't be reverting > those two files. https://codereview.chromium.org/896533003
https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... File base/profiler/cpu_profiler.h (right): https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler.h:5: remove line at 5 https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler.h:38: base::WaitableEvent waitable_event_; disable copy ctor and assignment https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler.h:81: DWORD io_thread_; are these two dwords the thread id? seems kind of racy, unless you assume they never die, which is a weird knowledge at the /base level. If it can't be a thread handle and it must be an id, then use xx_thread_id here and elsewhere and for the handle itself use xxx_thread or xx_thread_handle. https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler.h:95: Map params_; you can use auto, which should eliminate most needs for these kind of typedefs. https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... File base/profiler/cpu_profiler_win.cc (right): https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler_win.cc:29: const HMODULE hNtDll = ::GetModuleHandle(L"ntdll.dll"); hNTDLL does not follow the naming conventions https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler_win.cc:37: //SymSetOptions(SYMOPT_UNDNAME | SYMOPT_LOAD_LINES | SYMOPT_DEFERRED_LOADS); remove commented code. https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler_win.cc:57: } racy, you should open the thread handles and keep them, rather than the thread ids. Also it will be faster since you avoid OpenThread and CloseHandle being called all the time. https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler_win.cc:71: StackTraceEntry* stack) { this method does not need to be a member of the class https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler_win.cc:89: //debug::StackTrace trace(&thread_context); don't keep commented code. It is in or out. https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler_win.cc:117: if (strcmp(name, "CrBrowserMain") == 0) { I am afraid that this "CrBrowserMain" knowledge is not allowed at this layer, the user of the profiler needs to pass the names or better the thread ids. https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler_win.cc:136: for (int i = 0; i < stack_depth; i++, stack_trace++) { for ( ... , ++stack_trace) is the idiomatic use of our for loops. https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler_win.cc:148: if (GetModuleHandleEx(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS, careful with side effects for example here GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler_win.cc:154: RtlZeroMemory(&nvcontext, sizeof(KNONVOLATILE_CONTEXT_POINTERS)); 154 not needed, KNONVOLATILE_CONTEXT_POINTERS nvcontext = {0}; https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler_win.cc:162: context->Rip = (ULONG64)(*(PULONG64)context->Rsp); don't use C style casts. In general this code has the copied from msdn smell, please do a pass removing vestigial C code. https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler_win.cc:175: void CpuProfiler::GetNames(StackTraceEntry* stack_trace, int stack_depth) { seems also this one does not need to be part of the class, it can be standalone.
On 2015/02/03 02:40:54, cpu wrote: > https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... > File base/profiler/cpu_profiler.h (right): > > https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... > base/profiler/cpu_profiler.h:5: > remove line at 5 > > https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... > base/profiler/cpu_profiler.h:38: base::WaitableEvent waitable_event_; > disable copy ctor and assignment > > https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... > base/profiler/cpu_profiler.h:81: DWORD io_thread_; > are these two dwords the thread id? seems kind of racy, unless you assume they > never die, which is a weird knowledge at the /base level. > > If it can't be a thread handle and it must be an id, then use xx_thread_id here > and elsewhere and for the handle itself use xxx_thread or xx_thread_handle. > > https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... > base/profiler/cpu_profiler.h:95: Map params_; > you can use auto, which should eliminate most needs for these kind of typedefs. > > https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... > File base/profiler/cpu_profiler_win.cc (right): > > https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... > base/profiler/cpu_profiler_win.cc:29: const HMODULE hNtDll = > ::GetModuleHandle(L"ntdll.dll"); > hNTDLL does not follow the naming conventions > > https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... > base/profiler/cpu_profiler_win.cc:37: //SymSetOptions(SYMOPT_UNDNAME | > SYMOPT_LOAD_LINES | SYMOPT_DEFERRED_LOADS); > remove commented code. > > https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... > base/profiler/cpu_profiler_win.cc:57: } > racy, you should open the thread handles and keep them, rather than the thread > ids. Also it will be faster since you avoid OpenThread and CloseHandle being > called all the time. > > https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... > base/profiler/cpu_profiler_win.cc:71: StackTraceEntry* stack) { > this method does not need to be a member of the class > > https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... > base/profiler/cpu_profiler_win.cc:89: //debug::StackTrace > trace(&thread_context); > don't keep commented code. It is in or out. > > https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... > base/profiler/cpu_profiler_win.cc:117: if (strcmp(name, "CrBrowserMain") == 0) { > I am afraid that this "CrBrowserMain" knowledge is not allowed at this layer, > the user of the profiler needs to pass the names or better the thread ids. > > https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... > base/profiler/cpu_profiler_win.cc:136: for (int i = 0; i < stack_depth; i++, > stack_trace++) { > for ( ... , ++stack_trace) is the idiomatic use of our for loops. > > https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... > base/profiler/cpu_profiler_win.cc:148: if > (GetModuleHandleEx(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS, > careful with side effects for example here > GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT > > https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... > base/profiler/cpu_profiler_win.cc:154: RtlZeroMemory(&nvcontext, > sizeof(KNONVOLATILE_CONTEXT_POINTERS)); > 154 not needed, KNONVOLATILE_CONTEXT_POINTERS nvcontext = {0}; > > https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... > base/profiler/cpu_profiler_win.cc:162: context->Rip = > (ULONG64)(*(PULONG64)context->Rsp); > don't use C style casts. In general this code has the copied from msdn smell, > please do a pass removing vestigial C code. > > https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... > base/profiler/cpu_profiler_win.cc:175: void > CpuProfiler::GetNames(StackTraceEntry* stack_trace, int stack_depth) { > seems also this one does not need to be part of the class, it can be standalone. Ilya, the idea is that we can move faster with parallel approaches. The jank profiler informs us about the janky task. The sampling profiler can show us where in the task it is slow and potentially other interesting data. If you have more meta-concerns, let's take the discussion to the design document rather than this code review though.
+1. Sampling and "my" profilers complement each other. We can't throw away either one.
Alright, I'll defer to Vadim's judgement :) I just glanced over the code. Carlos, was there anything in particular that you wanted me to look at? Jim wrote most of the task-oriented profiler code; I just touched bits of it while piping the data to the UMA servers. https://codereview.chromium.org/888923004/diff/90001/base/profiler/cpu_profil... File base/profiler/cpu_profiler.h (right): https://codereview.chromium.org/888923004/diff/90001/base/profiler/cpu_profil... base/profiler/cpu_profiler.h:5: nit: Spurious newline. https://codereview.chromium.org/888923004/diff/90001/base/profiler/cpu_profil... base/profiler/cpu_profiler.h:39: }; Please move this class into a separate header + implementation file. https://codereview.chromium.org/888923004/diff/90001/base/profiler/cpu_profil... base/profiler/cpu_profiler.h:90: #endif I'd really prefer that you find a way to keep the Windows-specific parts of the interface outside of the shared header. https://codereview.chromium.org/888923004/diff/90001/base/profiler/cpu_profil... base/profiler/cpu_profiler.h:94: typedef std::map<std::string, std::string> Map; nit: I'm really not fond of this typedef -- especially given the super generic name. With C++11's support of the auto keyword, I think this sort of thing shouldn't be needed anymore. https://codereview.chromium.org/888923004/diff/90001/base/profiler/cpu_profil... base/profiler/cpu_profiler.h:101: } nit: This line should be "} // namespace base". Please fix this throughout. https://codereview.chromium.org/888923004/diff/90001/base/profiler/cpu_profil... base/profiler/cpu_profiler.h:103: nit: Spurious newline.
https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... File base/profiler/cpu_profiler.h (right): https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler.h:5: On 2015/02/03 02:40:53, cpu wrote: > remove line at 5 Done. https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler.h:38: base::WaitableEvent waitable_event_; On 2015/02/03 02:40:53, cpu wrote: > disable copy ctor and assignment Done. https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler.h:81: DWORD io_thread_; On 2015/02/03 02:40:53, cpu wrote: > are these two dwords the thread id? seems kind of racy, unless you assume they > never die, which is a weird knowledge at the /base level. > > If it can't be a thread handle and it must be an id, then use xx_thread_id here > and elsewhere and for the handle itself use xxx_thread or xx_thread_handle. > Changed to use a handle. https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler.h:95: Map params_; On 2015/02/03 02:40:53, cpu wrote: > you can use auto, which should eliminate most needs for these kind of typedefs. Done. https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... File base/profiler/cpu_profiler_win.cc (right): https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler_win.cc:29: const HMODULE hNtDll = ::GetModuleHandle(L"ntdll.dll"); On 2015/02/03 02:40:53, cpu wrote: > hNTDLL does not follow the naming conventions Done. https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler_win.cc:37: //SymSetOptions(SYMOPT_UNDNAME | SYMOPT_LOAD_LINES | SYMOPT_DEFERRED_LOADS); On 2015/02/03 02:40:53, cpu wrote: > remove commented code. Done. https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler_win.cc:57: } On 2015/02/03 02:40:53, cpu wrote: > racy, you should open the thread handles and keep them, rather than the thread > ids. Also it will be faster since you avoid OpenThread and CloseHandle being > called all the time. Done. https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler_win.cc:71: StackTraceEntry* stack) { On 2015/02/03 02:40:53, cpu wrote: > this method does not need to be a member of the class Done. https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler_win.cc:89: //debug::StackTrace trace(&thread_context); On 2015/02/03 02:40:53, cpu wrote: > don't keep commented code. It is in or out. Done. https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler_win.cc:117: if (strcmp(name, "CrBrowserMain") == 0) { On 2015/02/03 02:40:53, cpu wrote: > I am afraid that this "CrBrowserMain" knowledge is not allowed at this layer, > the user of the profiler needs to pass the names or better the thread ids. Changed this to just collect stacks from the main thread, whose handle we can capture in the constructor. This should give us enough information to evaluate performance. https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler_win.cc:136: for (int i = 0; i < stack_depth; i++, stack_trace++) { On 2015/02/03 02:40:53, cpu wrote: > for ( ... , ++stack_trace) is the idiomatic use of our for loops. Done. https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler_win.cc:148: if (GetModuleHandleEx(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS, On 2015/02/03 02:40:53, cpu wrote: > careful with side effects for example here > GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT Added a call to FreeLibrary, which should be sufficient for the purposes of testing performance. GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT looks up the module based on name. We need to look up based on IP. https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler_win.cc:154: RtlZeroMemory(&nvcontext, sizeof(KNONVOLATILE_CONTEXT_POINTERS)); On 2015/02/03 02:40:53, cpu wrote: > 154 not needed, KNONVOLATILE_CONTEXT_POINTERS nvcontext = {0}; Done. https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler_win.cc:162: context->Rip = (ULONG64)(*(PULONG64)context->Rsp); On 2015/02/03 02:40:53, cpu wrote: > don't use C style casts. In general this code has the copied from msdn smell, > please do a pass removing vestigial C code. Done. https://codereview.chromium.org/888923004/diff/70001/base/profiler/cpu_profil... base/profiler/cpu_profiler_win.cc:175: void CpuProfiler::GetNames(StackTraceEntry* stack_trace, int stack_depth) { On 2015/02/03 02:40:53, cpu wrote: > seems also this one does not need to be part of the class, it can be standalone. Done. https://codereview.chromium.org/888923004/diff/90001/base/profiler/cpu_profil... File base/profiler/cpu_profiler.h (right): https://codereview.chromium.org/888923004/diff/90001/base/profiler/cpu_profil... base/profiler/cpu_profiler.h:5: On 2015/02/03 22:12:10, Ilya Sherman wrote: > nit: Spurious newline. Done. https://codereview.chromium.org/888923004/diff/90001/base/profiler/cpu_profil... base/profiler/cpu_profiler.h:39: }; On 2015/02/03 22:12:10, Ilya Sherman wrote: > Please move this class into a separate header + implementation file. Can we keep this as-is for purposes of this commit? We're looking to validate this approach by landing for only as long as it takes to get performance measurements, and I'd prefer to limit the amount of restructuring required to do so. I will address these concerns before any attempt to land this code permanently. https://codereview.chromium.org/888923004/diff/90001/base/profiler/cpu_profil... base/profiler/cpu_profiler.h:90: #endif On 2015/02/03 22:12:10, Ilya Sherman wrote: > I'd really prefer that you find a way to keep the Windows-specific parts of the > interface outside of the shared header. Again, can we keep this as-is for purposes of this commit? I will address this before any future review of these changes. https://codereview.chromium.org/888923004/diff/90001/base/profiler/cpu_profil... base/profiler/cpu_profiler.h:94: typedef std::map<std::string, std::string> Map; On 2015/02/03 22:12:10, Ilya Sherman wrote: > nit: I'm really not fond of this typedef -- especially given the super generic > name. With C++11's support of the auto keyword, I think this sort of thing > shouldn't be needed anymore. Done. https://codereview.chromium.org/888923004/diff/90001/base/profiler/cpu_profil... base/profiler/cpu_profiler.h:101: } On 2015/02/03 22:12:10, Ilya Sherman wrote: > nit: This line should be "} // namespace base". Please fix this throughout. Done. https://codereview.chromium.org/888923004/diff/90001/base/profiler/cpu_profil... base/profiler/cpu_profiler.h:103: On 2015/02/03 22:12:09, Ilya Sherman wrote: > nit: Spurious newline. Done.
Ping... we're hoping to get perf numbers on this change soon since other work is gated on these results. Any chance of doing another round on this before EOD?
isherman@chromium.org changed reviewers: - isherman@chromium.org
https://codereview.chromium.org/888923004/diff/90001/base/profiler/cpu_profil... File base/profiler/cpu_profiler.h (right): https://codereview.chromium.org/888923004/diff/90001/base/profiler/cpu_profil... base/profiler/cpu_profiler.h:39: }; On 2015/02/04 01:37:10, Mike Wittman wrote: > On 2015/02/03 22:12:10, Ilya Sherman wrote: > > Please move this class into a separate header + implementation file. > > Can we keep this as-is for purposes of this commit? We're looking to validate > this approach by landing for only as long as it takes to get performance > measurements, and I'd prefer to limit the amount of restructuring required to do > so. I will address these concerns before any attempt to land this code > permanently. Ah, right, I recall now that you mentioned this was intended as temporary commit. That's fine by me, then.
mostly there. https://codereview.chromium.org/888923004/diff/110001/base/profiler/cpu_profi... File base/profiler/cpu_profiler_win.cc (right): https://codereview.chromium.org/888923004/diff/110001/base/profiler/cpu_profi... base/profiler/cpu_profiler_win.cc:42: if (GetModuleHandleEx(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS, I think or-ing both should work GET_MODULE_HANDLE_EX_UNCHANGED_REFCOUNT | GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS does it not? https://codereview.chromium.org/888923004/diff/110001/base/profiler/cpu_profi... base/profiler/cpu_profiler_win.cc:49: PVOID handler_data; pvoid --> void* https://codereview.chromium.org/888923004/diff/110001/base/profiler/cpu_profi... base/profiler/cpu_profiler_win.cc:71: if (!thread_handle) can we not fire the timer when you don't have a thread handle? https://codereview.chromium.org/888923004/diff/110001/base/profiler/cpu_profi... base/profiler/cpu_profiler_win.cc:98: PSYMBOL_INFO symbol_info = reinterpret_cast<PSYMBOL_INFO>(buffer); psymbol_info -->symbol_info* https://codereview.chromium.org/888923004/diff/110001/base/profiler/cpu_profi... base/profiler/cpu_profiler_win.cc:109: wcscpy(reinterpret_cast<wchar_t*>(symbol_info->Name), L"failed"); use the safe versions wcscpy_s https://codereview.chromium.org/888923004/diff/110001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/888923004/diff/110001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1000: std::map<std::string, std::string> params; you need a better name for this map, this is a thousand line function with many variables. I actually would prefer for Initialize() to take a pointer which can be null, given that clearly the params are optional.
https://codereview.chromium.org/888923004/diff/110001/base/profiler/cpu_profi... File base/profiler/cpu_profiler_win.cc (right): https://codereview.chromium.org/888923004/diff/110001/base/profiler/cpu_profi... base/profiler/cpu_profiler_win.cc:42: if (GetModuleHandleEx(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS, On 2015/02/05 01:03:26, cpu wrote: > I think or-ing both should work GET_MODULE_HANDLE_EX_UNCHANGED_REFCOUNT | > GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS > > does it not? > It seems that specifying GET_MODULE_HANDLE_EX_UNCHANGED_REFCOUNT either with or without GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS results in deadlocking the UI thread on startup, typically within a memory allocation or deallocation. I'll move the module lookup outside of the SuspendThread/ResumeThread section to avoid this entirely. https://codereview.chromium.org/888923004/diff/110001/base/profiler/cpu_profi... base/profiler/cpu_profiler_win.cc:49: PVOID handler_data; On 2015/02/05 01:03:27, cpu wrote: > pvoid --> void* Done. https://codereview.chromium.org/888923004/diff/110001/base/profiler/cpu_profi... base/profiler/cpu_profiler_win.cc:71: if (!thread_handle) On 2015/02/05 01:03:26, cpu wrote: > can we not fire the timer when you don't have a thread handle? We should always have a thread handle on x64 since we now get it from the main thread on construction of CpuProfiler. On x86 we never instantiate the SamplingThread. Removed the conditional. https://codereview.chromium.org/888923004/diff/110001/base/profiler/cpu_profi... base/profiler/cpu_profiler_win.cc:98: PSYMBOL_INFO symbol_info = reinterpret_cast<PSYMBOL_INFO>(buffer); On 2015/02/05 01:03:27, cpu wrote: > psymbol_info -->symbol_info* Done. https://codereview.chromium.org/888923004/diff/110001/base/profiler/cpu_profi... base/profiler/cpu_profiler_win.cc:109: wcscpy(reinterpret_cast<wchar_t*>(symbol_info->Name), L"failed"); On 2015/02/05 01:03:26, cpu wrote: > use the safe versions wcscpy_s Done. https://codereview.chromium.org/888923004/diff/110001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/888923004/diff/110001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1000: std::map<std::string, std::string> params; On 2015/02/05 01:03:27, cpu wrote: > you need a better name for this map, this is a thousand line function with many > variables. I actually would prefer for Initialize() to take a pointer which can > be null, given that clearly the params are optional. Changed to a pointer parameter.
cpu_profiler.h needs a long comment on the top about how it should be used. See src/base/timer/timer.h or src/base/callback.h for an extreme example. https://codereview.chromium.org/888923004/diff/130001/base/profiler/cpu_profi... File base/profiler/cpu_profiler.cc (right): https://codereview.chromium.org/888923004/diff/130001/base/profiler/cpu_profi... base/profiler/cpu_profiler.cc:41: Note that GetInstance() is not thread safe. As-is today it might not be very risky but as the code evolves it can be an issue. A way to mitigate this is to have sampling thread take in the ctor the CpuProfiler pointer. https://codereview.chromium.org/888923004/diff/130001/base/profiler/cpu_profi... base/profiler/cpu_profiler.cc:105: // end temp code ================== don't add a comment between single line else and the else, remove comment or add { }. frankly all code is temp code, so feel free to remove the two comments. https://codereview.chromium.org/888923004/diff/130001/base/profiler/cpu_profi... base/profiler/cpu_profiler.cc:133: const auto entry = params_.find(key); const auto& ? https://codereview.chromium.org/888923004/diff/130001/base/profiler/cpu_profi... base/profiler/cpu_profiler.cc:151: const auto entry = params_.find(key); same here I think you mean a reference. https://codereview.chromium.org/888923004/diff/130001/base/profiler/cpu_profi... File base/profiler/cpu_profiler.h (right): https://codereview.chromium.org/888923004/diff/130001/base/profiler/cpu_profi... base/profiler/cpu_profiler.h:49: please review the const-ness of each member function on this class, this is specially important since it appears can be accessed by multiple threads. https://codereview.chromium.org/888923004/diff/130001/base/profiler/cpu_profi... base/profiler/cpu_profiler.h:55: static void Stop(); wondering about Stop(), shouldn't it be a member instead of a static? https://codereview.chromium.org/888923004/diff/130001/base/profiler/cpu_profi... base/profiler/cpu_profiler.h:60: does OnTimer() need to be public? https://codereview.chromium.org/888923004/diff/130001/base/profiler/cpu_profi... base/profiler/cpu_profiler.h:85: scoped_ptr<SamplingThread> sampling_thread_; seems SamplingThread is an internal artifact. Is it not? If so you should move it to the cc file and have here just a fwd declaration.
On 2015/02/05 22:41:48, cpu wrote: > cpu_profiler.h needs a long comment on the top about how it should be used. > > See src/base/timer/timer.h or src/base/callback.h for an extreme example. Added a comment to CpuProfiler. I'm happy to provide an extended usage description once this is ready for general use, but that's not a goal at this point. We first need to establish that this technique is even feasible from a perf perspective. The intended purpose of this change is to do that by running it through the x64 perf bots, which requires a temporary commit since those bots are only available on the waterfall. https://codereview.chromium.org/888923004/diff/130001/base/profiler/cpu_profi... File base/profiler/cpu_profiler.cc (right): https://codereview.chromium.org/888923004/diff/130001/base/profiler/cpu_profi... base/profiler/cpu_profiler.cc:41: On 2015/02/05 22:41:47, cpu wrote: > Note that GetInstance() is not thread safe. > > As-is today it might not be very risky but as the code evolves it can be an > issue. A way to mitigate this is to have sampling thread take in the ctor the > CpuProfiler pointer. Done. https://codereview.chromium.org/888923004/diff/130001/base/profiler/cpu_profi... base/profiler/cpu_profiler.cc:105: // end temp code ================== On 2015/02/05 22:41:47, cpu wrote: > don't add a comment between single line else and the else, remove comment or add > { }. > > frankly all code is temp code, so feel free to remove the two comments. Done. https://codereview.chromium.org/888923004/diff/130001/base/profiler/cpu_profi... base/profiler/cpu_profiler.cc:133: const auto entry = params_.find(key); On 2015/02/05 22:41:47, cpu wrote: > const auto& ? If you prefer; not sure the additional iterator copy makes a huge difference here. https://codereview.chromium.org/888923004/diff/130001/base/profiler/cpu_profi... base/profiler/cpu_profiler.cc:151: const auto entry = params_.find(key); On 2015/02/05 22:41:47, cpu wrote: > same here I think you mean a reference. Done. https://codereview.chromium.org/888923004/diff/130001/base/profiler/cpu_profi... File base/profiler/cpu_profiler.h (right): https://codereview.chromium.org/888923004/diff/130001/base/profiler/cpu_profi... base/profiler/cpu_profiler.h:49: On 2015/02/05 22:41:47, cpu wrote: > please review the const-ness of each member function on this class, this is > specially important since it appears can be accessed by multiple threads. Done. https://codereview.chromium.org/888923004/diff/130001/base/profiler/cpu_profi... base/profiler/cpu_profiler.h:55: static void Stop(); On 2015/02/05 22:41:48, cpu wrote: > wondering about Stop(), shouldn't it be a member instead of a static? Yes, we can do away with the static instance entirely. https://codereview.chromium.org/888923004/diff/130001/base/profiler/cpu_profi... base/profiler/cpu_profiler.h:60: On 2015/02/05 22:41:48, cpu wrote: > does OnTimer() need to be public? No. Made it private. https://codereview.chromium.org/888923004/diff/130001/base/profiler/cpu_profi... base/profiler/cpu_profiler.h:85: scoped_ptr<SamplingThread> sampling_thread_; On 2015/02/05 22:41:47, cpu wrote: > seems SamplingThread is an internal artifact. Is it not? > > If so you should move it to the cc file and have here just a fwd declaration. Yes, done. https://codereview.chromium.org/888923004/diff/150001/base/profiler/cpu_profi... File base/profiler/cpu_profiler.cc (right): https://codereview.chromium.org/888923004/diff/150001/base/profiler/cpu_profi... base/profiler/cpu_profiler.cc:121: default_params[kSamplingSleepTime] = "100000"; // 100 ms FYI - I've updated these values to match the proposal in the design doc.
@cpu PTAL
lgtm I have other comments but lets go with this as-is for the purposes of measuring impact. The CL needs to stay for 6 hours if you want all perf bots to catch it. https://codereview.chromium.org/888923004/diff/130001/base/profiler/cpu_profi... File base/profiler/cpu_profiler.cc (right): https://codereview.chromium.org/888923004/diff/130001/base/profiler/cpu_profi... base/profiler/cpu_profiler.cc:133: const auto entry = params_.find(key); On 2015/02/06 20:01:14, Mike Wittman wrote: > On 2015/02/05 22:41:47, cpu wrote: > > const auto& ? > > If you prefer; not sure the additional iterator copy makes a huge difference > here. I misread, as is is fine. https://codereview.chromium.org/888923004/diff/150001/base/profiler/cpu_profi... File base/profiler/cpu_profiler.cc (right): https://codereview.chromium.org/888923004/diff/150001/base/profiler/cpu_profi... base/profiler/cpu_profiler.cc:121: default_params[kSamplingSleepTime] = "100000"; // 100 ms On 2015/02/06 20:01:15, Mike Wittman wrote: > FYI - I've updated these values to match the proposal in the design doc. Acknowledged.
New patchsets have been uploaded after l-g-t-m from cpu@chromium.org
The CQ bit was checked by wittman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/888923004/170001
Message was sent while issue was closed.
Committed patchset #10 (id:170001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/47c6093645b62fbc9f68cf50375fc0626e32516c Cr-Commit-Position: refs/heads/master@{#315196}
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:170001) has been created in https://codereview.chromium.org/904233002/ by wittman@chromium.org. The reason for reverting is: Reverting temporary commit.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
