Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1325653003:
Type change in StackSamplingProfiler from void* to uintptr_t. (Closed)
|
Created:
5 years, 3 months ago by sydli Modified:
5 years, 3 months ago Reviewers:
Mike Wittman, erikchen, cpu_(ooo_6.6-7.5), Avi (use Gerrit), robliao, Alexei Svitkine (slow), Tom Sepez CC:
chromium-reviews, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@UMA2_refactor Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAll instances of addresses as void* are changed to uintptr_t in
StackSamplingProfiler. These addresses should not be dereferenced, so
uintptr_t is a better type for recorded instruction pointers. Additionally,
this change will allow IPC macros to use these structures in the future.
BUG=517958
Committed: https://crrev.com/f192d05e967d328c2118a6b5636c09c80a051c97
Cr-Commit-Position: refs/heads/master@{#349254}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Split type change into separate CL and addressed comments. #Patch Set 3 : void* --> uintptr_t #Patch Set 4 : Accidentally removed const qualifiers from unittest. #
Total comments: 5
Patch Set 5 : Moved IPC structures into content/common and merged with dependent CL. #
Total comments: 13
Patch Set 6 : Addressed wittman's comments. #
Total comments: 6
Patch Set 7 : Nits and compiles. #
Total comments: 1
Patch Set 8 : Rebase and CL now only includes type changes. #
Messages
Total messages: 38 (7 generated)
sydli@google.com changed reviewers: + erikchen@chromium.org, wittman@chromium.org
+erikchen and +wittman for review. PTAL-- especially unsure about uintptr_t casts made to satisfy IPC macros.
> messages for stack sampling profiler to sample on non-browser processes, > then send collected profiles to browser process for upload to UMA. Empty > message handler created in profile_message_filter for future use. Descriptions should be full sentences with proper capitalization. https://codereview.chromium.org/1325653003/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1325653003/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:72: uintptr_t base_address; I'd prefer to keep the const void* type for this and instruction_pointer. This makes debugging the profiler a *lot* easier since the Visual Studio debugger automatically symbolizes these when explicitly represented as pointer values. I think this should be possible by creating an IPC::ParamTraits template specialization for Module and Frame.
https://codereview.chromium.org/1325653003/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1325653003/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:72: uintptr_t base_address; On 2015/08/31 20:57:57, Mike Wittman wrote: > I'd prefer to keep the const void* type for this and instruction_pointer. This > makes debugging the profiler a *lot* easier since the Visual Studio debugger > automatically symbolizes these when explicitly represented as pointer values. > > I think this should be possible by creating an IPC::ParamTraits template > specialization for Module and Frame. The type of this member should not be dependent on Visual Studio capabilities. Back when this class was only ever instantiated/used in a single process, const void* was the correct type. Since this class is going to be passed across Chrome IPC, const void* is no longer the correct type (and is even dangerous, since one might be tempted to dereference the pointer in the receiving process).
lgtm with nits https://codereview.chromium.org/1325653003/diff/1/content/browser/profiler_me... File content/browser/profiler_message_filter.h (right): https://codereview.chromium.org/1325653003/diff/1/content/browser/profiler_me... content/browser/profiler_message_filter.h:11: #include "base/profiler/stack_sampling_profiler.h" Should we forward declare the types we need and add the includes to the .cc file?
https://codereview.chromium.org/1325653003/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1325653003/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:67: Module(const void* base_address, const std::string& id, The |base_address| parameter should use the same type as is stored in the struct. https://codereview.chromium.org/1325653003/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:72: uintptr_t base_address; On 2015/08/31 21:15:17, erikchen wrote: > On 2015/08/31 20:57:57, Mike Wittman wrote: > > I'd prefer to keep the const void* type for this and instruction_pointer. This > > makes debugging the profiler a *lot* easier since the Visual Studio debugger > > automatically symbolizes these when explicitly represented as pointer values. > > > > I think this should be possible by creating an IPC::ParamTraits template > > specialization for Module and Frame. > > The type of this member should not be dependent on Visual Studio capabilities. Debuggability of the code is always a relevant concern, and that code runs only on Windows. > Back when this class was only ever instantiated/used in a single process, const > void* was the correct type. > > Since this class is going to be passed across Chrome IPC, const void* is no > longer the correct type (and is even dangerous, since one might be tempted to > dereference the pointer in the receiving process). However, I agree that representing as pointers in a different process is misleading at best. Unfortunately I don't see a good way to preserve the pointer representation in the browser process while supporting uintptr_t for IPC, so uintptr_t it is. I'll provide additional comments accordingly. https://codereview.chromium.org/1325653003/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:93: Frame(const void* instruction_pointer, size_t module_index); Similarly, the |instruction_pointer| parameter should use the same type as is stored in the struct.
Worried that uintptr_t change might affect functionality, so broke it off into a separate CL: https://codereview.chromium.org/1319973011/ Currently testing this on Windows machine. https://codereview.chromium.org/1325653003/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1325653003/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:67: Module(const void* base_address, const std::string& id, On 2015/08/31 21:42:00, Mike Wittman wrote: > The |base_address| parameter should use the same type as is stored in the > struct. Done. https://codereview.chromium.org/1325653003/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:72: uintptr_t base_address; On 2015/08/31 21:42:00, Mike Wittman wrote: > On 2015/08/31 21:15:17, erikchen wrote: > > On 2015/08/31 20:57:57, Mike Wittman wrote: > > > I'd prefer to keep the const void* type for this and instruction_pointer. > This > > > makes debugging the profiler a *lot* easier since the Visual Studio debugger > > > automatically symbolizes these when explicitly represented as pointer > values. > > > > > > I think this should be possible by creating an IPC::ParamTraits template > > > specialization for Module and Frame. > > > > The type of this member should not be dependent on Visual Studio capabilities. > > Debuggability of the code is always a relevant concern, and that code runs only > on Windows. > > > Back when this class was only ever instantiated/used in a single process, > const > > void* was the correct type. > > > > Since this class is going to be passed across Chrome IPC, const void* is no > > longer the correct type (and is even dangerous, since one might be tempted to > > dereference the pointer in the receiving process). > > However, I agree that representing as pointers in a different process is > misleading at best. > > Unfortunately I don't see a good way to preserve the pointer representation in > the browser process while supporting uintptr_t for IPC, so uintptr_t it is. I'll > provide additional comments accordingly. Okay; I split this into a separate CL: https://codereview.chromium.org/1319973011/ in case any functionality of the profiler breaks with the uintptr_t change (Don't think it should, but just in case). I'll run the related tests on a Windows machine. https://codereview.chromium.org/1325653003/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:93: Frame(const void* instruction_pointer, size_t module_index); On 2015/08/31 21:42:00, Mike Wittman wrote: > Similarly, the |instruction_pointer| parameter should use the same type as is > stored in the struct. Done. https://codereview.chromium.org/1325653003/diff/1/content/browser/profiler_me... File content/browser/profiler_message_filter.h (right): https://codereview.chromium.org/1325653003/diff/1/content/browser/profiler_me... content/browser/profiler_message_filter.h:11: #include "base/profiler/stack_sampling_profiler.h" On 2015/08/31 21:17:28, erikchen wrote: > Should we forward declare the types we need and add the includes to the .cc > file? Done. Couldn't forward declare CallStackProfiles, though.
https://codereview.chromium.org/1325653003/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/1325653003/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:72: uintptr_t base_address; On 2015/09/01 00:29:59, sydli wrote: > On 2015/08/31 21:42:00, Mike Wittman wrote: > > On 2015/08/31 21:15:17, erikchen wrote: > > > On 2015/08/31 20:57:57, Mike Wittman wrote: > > > > I'd prefer to keep the const void* type for this and instruction_pointer. > > This > > > > makes debugging the profiler a *lot* easier since the Visual Studio > debugger > > > > automatically symbolizes these when explicitly represented as pointer > > values. > > > > > > > > I think this should be possible by creating an IPC::ParamTraits template > > > > specialization for Module and Frame. > > > > > > The type of this member should not be dependent on Visual Studio > capabilities. > > > > Debuggability of the code is always a relevant concern, and that code runs > only > > on Windows. > > > > > Back when this class was only ever instantiated/used in a single process, > > const > > > void* was the correct type. > > > > > > Since this class is going to be passed across Chrome IPC, const void* is no > > > longer the correct type (and is even dangerous, since one might be tempted > to > > > dereference the pointer in the receiving process). > > > > However, I agree that representing as pointers in a different process is > > misleading at best. > > > > Unfortunately I don't see a good way to preserve the pointer representation in > > the browser process while supporting uintptr_t for IPC, so uintptr_t it is. > I'll > > provide additional comments accordingly. > > Okay; I split this into a separate CL: > https://codereview.chromium.org/1319973011/ > in case any functionality of the profiler breaks with the uintptr_t change > (Don't think it should, but just in case). I'll run the related tests on a > Windows machine. The const void* => uintptr_t change should be pretty low risk. If it compiles and there's no obvious issues in the code review, it should be fine. Since we don't want pointers in the IPC representation, I think we should keep that change in this CL.
On 2015/09/01 00:41:57, Mike Wittman wrote: > https://codereview.chromium.org/1325653003/diff/1/base/profiler/stack_samplin... > File base/profiler/stack_sampling_profiler.h (right): > > https://codereview.chromium.org/1325653003/diff/1/base/profiler/stack_samplin... > base/profiler/stack_sampling_profiler.h:72: uintptr_t base_address; > On 2015/09/01 00:29:59, sydli wrote: > > On 2015/08/31 21:42:00, Mike Wittman wrote: > > > On 2015/08/31 21:15:17, erikchen wrote: > > > > On 2015/08/31 20:57:57, Mike Wittman wrote: > > > > > I'd prefer to keep the const void* type for this and > instruction_pointer. > > > This > > > > > makes debugging the profiler a *lot* easier since the Visual Studio > > debugger > > > > > automatically symbolizes these when explicitly represented as pointer > > > values. > > > > > > > > > > I think this should be possible by creating an IPC::ParamTraits template > > > > > specialization for Module and Frame. > > > > > > > > The type of this member should not be dependent on Visual Studio > > capabilities. > > > > > > Debuggability of the code is always a relevant concern, and that code runs > > only > > > on Windows. > > > > > > > Back when this class was only ever instantiated/used in a single process, > > > const > > > > void* was the correct type. > > > > > > > > Since this class is going to be passed across Chrome IPC, const void* is > no > > > > longer the correct type (and is even dangerous, since one might be tempted > > to > > > > dereference the pointer in the receiving process). > > > > > > However, I agree that representing as pointers in a different process is > > > misleading at best. > > > > > > Unfortunately I don't see a good way to preserve the pointer representation > in > > > the browser process while supporting uintptr_t for IPC, so uintptr_t it is. > > I'll > > > provide additional comments accordingly. > > > > Okay; I split this into a separate CL: > > https://codereview.chromium.org/1319973011/ > > in case any functionality of the profiler breaks with the uintptr_t change > > (Don't think it should, but just in case). I'll run the related tests on a > > Windows machine. > > The const void* => uintptr_t change should be pretty low risk. If it compiles > and there's no obvious issues in the code review, it should be fine. > > Since we don't want pointers in the IPC representation, I think we should keep > that change in this CL. Ok, sounds good. Combined CLs.
assuming this compiles, lgtm with a few nits https://codereview.chromium.org/1325653003/diff/60001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/1325653003/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.cc:87: : base_address(reinterpret_cast<uintptr_t>(nullptr)) {} nit: just base_address(0u) https://codereview.chromium.org/1325653003/diff/60001/components/metrics/call... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/1325653003/diff/60001/components/metrics/call... components/metrics/call_stack_profile_metrics_provider_unittest.cc:470: 0x1000, Frame::kUnknownModuleIndex))); nit: move first arg to previous line (and second, if possible) (and below instances)
sydli@google.com changed reviewers: + robliao@chromium.org
Since there's no need for a base/ refactor, moved that CL into this one. Defined structures for IPC in profiled_stack_state. PTAL? +robliao since he's back :) https://codereview.chromium.org/1325653003/diff/60001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/1325653003/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.cc:87: : base_address(reinterpret_cast<uintptr_t>(nullptr)) {} On 2015/09/01 01:42:15, Mike Wittman wrote: > nit: just base_address(0u) Done. https://codereview.chromium.org/1325653003/diff/60001/components/metrics/call... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/1325653003/diff/60001/components/metrics/call... components/metrics/call_stack_profile_metrics_provider_unittest.cc:470: 0x1000, Frame::kUnknownModuleIndex))); On 2015/09/01 01:42:15, Mike Wittman wrote: > nit: move first arg to previous line (and second, if possible) > > (and below instances) Ran git cl format.
https://codereview.chromium.org/1325653003/diff/60001/components/metrics/call... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/1325653003/diff/60001/components/metrics/call... components/metrics/call_stack_profile_metrics_provider_unittest.cc:470: 0x1000, Frame::kUnknownModuleIndex))); On 2015/09/03 16:21:15, sydli wrote: > On 2015/09/01 01:42:15, Mike Wittman wrote: > > nit: move first arg to previous line (and second, if possible) > > > > (and below instances) > > Ran git cl format. This is fine for this change and below, but has introduced a number of unnecessary formatting changes above this that make it harder to see what has changed. Can you undo the reformat of all the places I've noted? https://codereview.chromium.org/1325653003/diff/80001/components/metrics/call... File components/metrics/call_stack_profile_metrics_provider.h (right): https://codereview.chromium.org/1325653003/diff/80001/components/metrics/call... components/metrics/call_stack_profile_metrics_provider.h:21: // Keep this enum in sync with content/common/profiled_stack_state.h nit: we generally don't use imperative comments in Chrome; maybe "This enum must be kept in sync with" instead (and the other Trigger location) https://codereview.chromium.org/1325653003/diff/80001/components/metrics/call... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/1325653003/diff/80001/components/metrics/call... components/metrics/call_stack_profile_metrics_provider_unittest.cc:66: { Please undo reformatting (see "git cl format" comment). https://codereview.chromium.org/1325653003/diff/80001/components/metrics/call... components/metrics/call_stack_profile_metrics_provider_unittest.cc:138: {{Frame(module1_base_address + 0x10, 0), Please undo reformatting. https://codereview.chromium.org/1325653003/diff/80001/components/metrics/call... components/metrics/call_stack_profile_metrics_provider_unittest.cc:256: Module(module_base_address, "ABCD", Please undo reformatting. https://codereview.chromium.org/1325653003/diff/80001/components/metrics/call... components/metrics/call_stack_profile_metrics_provider_unittest.cc:266: const Frame sample_frames[][1] = {{ Please undo reformatting. https://codereview.chromium.org/1325653003/diff/80001/components/metrics/call... components/metrics/call_stack_profile_metrics_provider_unittest.cc:337: Module(module_base_address, "ABCD", Please undo reformatting. https://codereview.chromium.org/1325653003/diff/80001/components/metrics/call... components/metrics/call_stack_profile_metrics_provider_unittest.cc:347: const Frame sample_frames[][1] = {{ Please undo reformatting. https://codereview.chromium.org/1325653003/diff/80001/content/common/profiled... File content/common/profiled_stack_state.h (right): https://codereview.chromium.org/1325653003/diff/80001/content/common/profiled... content/common/profiled_stack_state.h:12: class TimeTicks; This should be an include. Forward declaration is only valid for types used by pointer or reference, not by value. This happens to work because base/profiler/stack_sampling_profiler.h also includes base/time/time.h. :) https://codereview.chromium.org/1325653003/diff/80001/content/common/profiled... content/common/profiled_stack_state.h:36: // Default constructor and destrucor exposed to satisfy IPC macros. nit: this should only refer to the constructor; use of the destructor is unavoidable.
Done-- sorry about the format. Should I reformat after reviews? https://codereview.chromium.org/1325653003/diff/80001/components/metrics/call... File components/metrics/call_stack_profile_metrics_provider.h (right): https://codereview.chromium.org/1325653003/diff/80001/components/metrics/call... components/metrics/call_stack_profile_metrics_provider.h:21: // Keep this enum in sync with content/common/profiled_stack_state.h On 2015/09/03 16:58:04, Mike Wittman wrote: > nit: we generally don't use imperative comments in Chrome; maybe "This enum must > be kept in sync with" instead > > (and the other Trigger location) Done. https://codereview.chromium.org/1325653003/diff/80001/components/metrics/call... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/1325653003/diff/80001/components/metrics/call... components/metrics/call_stack_profile_metrics_provider_unittest.cc:66: { On 2015/09/03 16:58:04, Mike Wittman wrote: > Please undo reformatting (see "git cl format" comment). Gotcha. Unreformatted. https://codereview.chromium.org/1325653003/diff/80001/content/common/profiled... File content/common/profiled_stack_state.h (right): https://codereview.chromium.org/1325653003/diff/80001/content/common/profiled... content/common/profiled_stack_state.h:12: class TimeTicks; On 2015/09/03 16:58:04, Mike Wittman wrote: > This should be an include. Forward declaration is only valid for types used by > pointer or reference, not by value. This happens to work because > base/profiler/stack_sampling_profiler.h also includes base/time/time.h. :) Ah, that makes sense. Done. https://codereview.chromium.org/1325653003/diff/80001/content/common/profiled... content/common/profiled_stack_state.h:36: // Default constructor and destrucor exposed to satisfy IPC macros. On 2015/09/03 16:58:04, Mike Wittman wrote: > nit: this should only refer to the constructor; use of the destructor is > unavoidable. Done.
lgtm with nit with the latest changes On 2015/09/03 17:18:13, sydli wrote: > Done-- sorry about the format. Should I reformat after reviews? Running git cl format is fine, but sometimes it requires a little supervision. :) It's virtually certain to want to reformat any extended initializers, often not for the best. https://codereview.chromium.org/1325653003/diff/100001/content/common/profile... File content/common/profiled_stack_state.h (right): https://codereview.chromium.org/1325653003/diff/100001/content/common/profile... content/common/profiled_stack_state.h:33: // Default constructor and exposed to satisfy IPC macros. nit: is exposed
lgtm https://codereview.chromium.org/1325653003/diff/100001/base/profiler/native_s... File base/profiler/native_stack_sampler_win.cc (right): https://codereview.chromium.org/1325653003/diff/100001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:308: reintrepret_cast<uintptr_t>(instruction_pointers[i]), reint[re]pret -> reinterpret
https://codereview.chromium.org/1325653003/diff/100001/content/common/profile... File content/common/profiled_stack_state.h (right): https://codereview.chromium.org/1325653003/diff/100001/content/common/profile... content/common/profiled_stack_state.h:35: ProfiledStackState(); You can make this private with a friend class, but it's ugly. I'd say avoid for now since no one else does this: private: friend struct base::TupleLeaf<0, ProfiledStackState>; // Default constructor and exposed to satisfy IPC macros. // Do not use explicitly. ProfiledStackState(); This gets picked up through the IPC message declaration macros.
sydli@google.com changed reviewers: + asvitkine@chromium.org, avi@chromium.org, cpu@chromium.org, tsepez@chromium.org
Nits. Tests look green so sending out for OWNER review. +cpu for base/ changes, +asvitkine for components/, +tsepez for IPC messages and security, and +avi for content/. https://codereview.chromium.org/1325653003/diff/100001/base/profiler/native_s... File base/profiler/native_stack_sampler_win.cc (right): https://codereview.chromium.org/1325653003/diff/100001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:308: reintrepret_cast<uintptr_t>(instruction_pointers[i]), On 2015/09/03 21:46:45, robliao wrote: > reint[re]pret -> reinterpret Done. https://codereview.chromium.org/1325653003/diff/100001/content/common/profile... File content/common/profiled_stack_state.h (right): https://codereview.chromium.org/1325653003/diff/100001/content/common/profile... content/common/profiled_stack_state.h:33: // Default constructor and exposed to satisfy IPC macros. On 2015/09/03 18:16:36, Mike Wittman wrote: > nit: is exposed Done. https://codereview.chromium.org/1325653003/diff/100001/content/common/profile... content/common/profiled_stack_state.h:35: ProfiledStackState(); On 2015/09/03 22:25:59, robliao wrote: > You can make this private with a friend class, but it's ugly. I'd say avoid for > now since no one else does this: > > private: > friend struct base::TupleLeaf<0, ProfiledStackState>; > // Default constructor and exposed to satisfy IPC macros. > // Do not use explicitly. > ProfiledStackState(); > > This gets picked up through the IPC message declaration macros. Thanks for looking into this!
lgtm https://codereview.chromium.org/1325653003/diff/120001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/1325653003/diff/120001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.cc:87: StackSamplingProfiler::Module::Module(const uintptr_t base_address, Nit: Remove the const qualifier.
Shouldn't there be a corresponding GN file for the gyp changes? Messages LGTM. Please CC me on the CL that actually processes your new message
On 2015/09/09 17:26:36, Tom Sepez wrote: > Shouldn't there be a corresponding GN file for the gyp changes? Looks like they're added to GN build via the script here: https://code.google.com/p/chromium/codesearch#chromium/src/content/common/com...
lgtm
drive-by: this is a (google) chrome feature. why is there code in content? one problem with reviewing such minimal changes is that they don't show how they will be called. i.e. what code will consume these new data structures?
On 2015/09/14 17:22:50, jam wrote: > drive-by: this is a (google) chrome feature. why is there code in content? > > one problem with reviewing such minimal changes is that they don't show how they > will be called. i.e. what code will consume these new data structures? It's a chrome feature, but we're looking to profile stacks on non-browser (child) processes. Right now trying to land CLs to profile GPU startup; the dependent CLs are linked above. Since both child process messages and GPU Main live in content/, quite a few of the related changes have been in content/. Related designdoc here: https://docs.google.com/document/d/1uEwJh-R1SZl86V4Irk0iS7diZN5R4QbqqGRtg1WYT...
On 2015/09/14 17:31:35, sydli wrote: > On 2015/09/14 17:22:50, jam wrote: > > drive-by: this is a (google) chrome feature. why is there code in content? > > > > one problem with reviewing such minimal changes is that they don't show how > they > > will be called. i.e. what code will consume these new data structures? > > It's a chrome feature, but we're looking to profile stacks on non-browser > (child) processes. Right now trying to land CLs to profile GPU startup; the > dependent CLs are linked above. > Since both child process messages and GPU Main live in content/, quite a few of > the related changes have been in content/. these are two orthogonal things. i.e. the fundamental renderer code is in content. but then chrome builds upon it and adds its features there. same for gpu, chrome layer should be able to add IPC observers and send/respond to IPCs there. see https://www.chromium.org/developers/content-module > Related designdoc here: > https://docs.google.com/document/d/1uEwJh-R1SZl86V4Irk0iS7diZN5R4QbqqGRtg1WYT...
base/ code lgtm. Please make sure jam@ is ok with your approach before landing. Alternatively you can split the CL in two, one with the base changes and one with profiled_stack_state stuff.
On 2015/09/14 20:59:33, cpu wrote: > base/ code lgtm. Please make sure jam@ is ok with your approach before landing. > Alternatively you can split the CL in two, one with the base changes and one > with profiled_stack_state stuff. Yep! Currently working on another approach with jam. I filtered the other changes from this CL and will land them separately. This CL is now just a type change (const void* -> uintptr_t).
The CQ bit was checked by sydli@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from erikchen@chromium.org, wittman@chromium.org, robliao@chromium.org, cpu@chromium.org, avi@chromium.org, asvitkine@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/1325653003/#ps140001 (title: "Rebase and CL now only includes type changes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1325653003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1325653003/140001
The CQ bit was unchecked by sydli@google.com
On 2015/09/16 19:15:42, sydli wrote: > On 2015/09/14 20:59:33, cpu wrote: > > base/ code lgtm. Please make sure jam@ is ok with your approach before > landing. > > Alternatively you can split the CL in two, one with the base changes and one > > with profiled_stack_state stuff. > > Yep! Currently working on another approach with jam. I filtered the other > changes from this CL and will land them separately. > This CL is now just a type change (const void* -> uintptr_t). Can you update the CL title and description since it no longer matches the content?
On 2015/09/16 19:21:03, Mike Wittman wrote: > On 2015/09/16 19:15:42, sydli wrote: > > On 2015/09/14 20:59:33, cpu wrote: > > > base/ code lgtm. Please make sure jam@ is ok with your approach before > > landing. > > > Alternatively you can split the CL in two, one with the base changes and one > > > with profiled_stack_state stuff. > > > > Yep! Currently working on another approach with jam. I filtered the other > > changes from this CL and will land them separately. > > This CL is now just a type change (const void* -> uintptr_t). > > Can you update the CL title and description since it no longer matches the > content? Updated.
The CQ bit was checked by sydli@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1325653003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1325653003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/f192d05e967d328c2118a6b5636c09c80a051c97 Cr-Commit-Position: refs/heads/master@{#349254} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
