Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1839503002:
[tracing] Add native allocation tracing mode. (Closed)
|
Created:
4 years, 9 months ago by Dmitry Skiba Modified:
4 years, 8 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, Dai Mikurube (NOT FULLTIME), ssid Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tracing] Add native allocation tracing mode.
This change adds 'native' mode to '--enable-heap-profiling' option. When
started with '--enable-heap-profiling=native' Chrome will keep native
(real) stack traces for all live allocations. These native stack traces
are then dumped to a trace file if 'memory-infra' category is enabled.
Stack traces are dumped unsymbolized (for performance reasons), and there
is a script (third_party/catapult/tracing/bin/symbolize_trace) that
symbolizes all stack traces in a given trace file.
Design document: https://goo.gl/DFoqfi
Note: native stack tracing relies on frame pointers. You'll need to use
either debug or a profiling release build (enable_profiling = true).
Note: for now this all works on Linux @ x86/x64 only.
BUG=602701
Committed: https://crrev.com/5a05938adaabf413a40f1e0dfec3d1e4c683693a
Cr-Commit-Position: refs/heads/master@{#389500}
Patch Set 1 #Patch Set 2 : Fix startup tracing #Patch Set 3 : WalkStackFrames (wants frame pointers) #
Total comments: 13
Patch Set 4 : rebase #Patch Set 5 : --enable-heap-profiling=native; misc cleanups #Patch Set 6 : Add type to StackFrame; format thread name #
Total comments: 30
Patch Set 7 : Address comments; fix unittests #Patch Set 8 : rebase #Patch Set 9 : rebase #Patch Set 10 : Revert whitespace change in malloc_dump_provider #
Total comments: 15
Patch Set 11 : Address comments #
Total comments: 11
Patch Set 12 : rebase #Patch Set 13 : Address comment #Patch Set 14 : rebase #Patch Set 15 : Increase Backtrace frames, request even more frames #Depends on Patchset: Messages
Total messages: 45 (19 generated)
Description was changed from ========== Implement experimental allocation tracing. When enabled, stack trace for each allocation is recorded. Traces are unsymbolized, use tools/trace/symbolize.py to symbolize them. Build with (GN only for now): use_experimental_allocation_tracing = true enable_full_stack_frames_for_profiling = true Run Chrome with --enable-heap-profiling flag. BUG= ========== to ========== Implement experimental allocation tracing. When enabled, stack trace for each allocation is recorded. Resulting trace file contains unsymbolized stack frames, and there is a script tools/trace/symbolize.py to symbolize them. Build with (GN only for now): use_experimental_allocation_tracing = true enable_full_stack_frames_for_profiling = true Run Chrome with --enable-heap-profiling flag. BUG= ==========
Description was changed from ========== Implement experimental allocation tracing. When enabled, stack trace for each allocation is recorded. Resulting trace file contains unsymbolized stack frames, and there is a script tools/trace/symbolize.py to symbolize them. Build with (GN only for now): use_experimental_allocation_tracing = true enable_full_stack_frames_for_profiling = true Run Chrome with --enable-heap-profiling flag. BUG= ========== to ========== Implement experimental allocation tracing. When enabled, stack trace for each allocation is recorded. Resulting trace file contains unsymbolized stack frames, and there is a script tools/trace/symbolize.py to symbolize them. Build with (GN only for now): use_experimental_allocation_tracing = true enable_full_stack_frames_for_profiling = true Run Chrome with --enable-heap-profiling flag. Single "memory-infra" category is enough (pseudo stack traces from other categories are ignored). BUG= ==========
dskiba@google.com changed reviewers: + primiano@chromium.org
Description was changed from ========== Implement experimental allocation tracing. When enabled, stack trace for each allocation is recorded. Resulting trace file contains unsymbolized stack frames, and there is a script tools/trace/symbolize.py to symbolize them. Build with (GN only for now): use_experimental_allocation_tracing = true enable_full_stack_frames_for_profiling = true Run Chrome with --enable-heap-profiling flag. Single "memory-infra" category is enough (pseudo stack traces from other categories are ignored). BUG= ========== to ========== Implement experimental allocation tracing. When enabled, stack trace for each allocation is recorded. Resulting trace file contains unsymbolized stack frames, and there is a script tools/trace/symbolize.py to symbolize them. Build with GN: use_experimental_allocation_tracing = true enable_profiling = true enable_nacl = false enable_full_stack_frames_for_profiling = true Run Chrome with --enable-heap-profiling flag. Trace with "memory-infra" category. BUG= ==========
Thanks a lot for looking at this. this was quick. the approach from the tracing codebase looks very reasonable, I have some high level questions first: 1) Would be nice if we could avoid yet another build flag (build owners might be recluctant to that). If you think to that, do you really need that? I think we could just recycle the existing run-time switch --enable-heap-profiling and say that you can pass an optional =native argument (--enable-heap-profiling=native), which enables this new mode. 2) I see that you are adding a type to each frame. Do you really want to support the case of hybrid traces where you can mix both native and pseudo stacks? Or should that information be just global (i.e. all frames in the trace are pseudo|native). I'd probably lean towards the 2nd as it's easier to maintain, unless you feeel there is a good use case for mixed ones. 3) Instead of recycling pieces of tcmalloc, should we make just that available to use? In other words... see my question inline below: if you could use GetStackFrame from tcmalloc/gperfetools.h would that be good for you? If so we can make a case and always link that part of tcmalloc, provided that isn't too big in terms of binary (doesn't looks so) https://codereview.chromium.org/1839503002/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context.h (right): https://codereview.chromium.org/1839503002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.h:40: enum StackFrameType { do we really need this run-time information per each frame? In other words, do we ever want to support the case where some stack frames are native and others are pseudo? Or should that be just a global setting? https://codereview.chromium.org/1839503002/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1839503002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:42: size_t WalkStackFrames(const void** trace, would be nice to make this available as a general base feature and just fix base::debug::StackTrace somehow. I wonder, if you could include and use GetStackFrames from tcmalloc would you still need this function? If so we can have some discussion with base owners. I wonder if the right thing to do here is making a subset of tcmalloc always avalable (only the gperftools part) so that you can use the tcmalloc code for stack unwinding even if you are not using tcmalloc as an allocator? https://codereview.chromium.org/1839503002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:130: // allocate memory during initialization (e.g. backtrace() does that). where is backtrace() called from? https://codereview.chromium.org/1839503002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:179: std::reverse(ctx.backtrace.frames, I wonder if we could avoid this reverse and just walk backwards in heap_profiler_stack_frame_deduplicator.cc when #ifdef BLA, to avoid this reverse oepration on each malloc() (Could do in a separate CL, maybe just add a todo w/ a crbug) https://codereview.chromium.org/1839503002/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.cc (right): https://codereview.chromium.org/1839503002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:74: const StackFrame* begin = std::begin(backtrace.frames); if StackFrame is "const char*", I think const char* const* begin should become: StackFrame const* begin same below https://codereview.chromium.org/1839503002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:239: if (size != 0) { uh do we see any zero allocations? https://codereview.chromium.org/1839503002/diff/40001/tools/trace/symbolize.py File tools/trace/symbolize.py (right): https://codereview.chromium.org/1839503002/diff/40001/tools/trace/symbolize.p... tools/trace/symbolize.py:1: #!/usr/bin/env python not sure whether this is the right folder for this script. It seems to contain just two files. I guess this would be better placed in catapult (and we'd have to copy the elf_symbolizer.py which is standalone there) Let's discuss this with perezju@ who might have a better idea.
https://codereview.chromium.org/1839503002/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context.h (right): https://codereview.chromium.org/1839503002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.h:40: enum StackFrameType { On 2016/04/01 15:56:28, Primiano Tucci wrote: > do we really need this run-time information per each frame? > In other words, do we ever want to support the case where some stack frames are > native and others are pseudo? > Or should that be just a global setting? It's called that way, but it's used per backtrace, not per frame. I think that we should go another direction - introduce it per-frame, so that we could for example add things like thread names to native stack traces. And other way around too - Sid has an idea to add addresses of task functions to pseudo stack traces. Those addresses always available, and provide precise place where task-related allocations stem from. https://codereview.chromium.org/1839503002/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1839503002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:42: size_t WalkStackFrames(const void** trace, On 2016/04/01 15:56:28, Primiano Tucci wrote: > would be nice to make this available as a general base feature and just fix > base::debug::StackTrace somehow. > I wonder, if you could include and use GetStackFrames from tcmalloc would you > still need this function? > If so we can have some discussion with base owners. > I wonder if the right thing to do here is making a subset of tcmalloc always > avalable (only the gperftools part) so that you can use the tcmalloc code for > stack unwinding even if you are not using tcmalloc as an allocator? Yes, I would prefer to make it available through StackTrace. I don't think we need to try to bring TCMalloc though - the function is simple and TCMalloc's version supports way more stuff than we need (and it's kinda rusty, supporting gcc 4.1, etc). My plan was to first have something stable here, and then move it to StackTrace. But I can do it other way around - first figure out StackTrace interface for it, add it there, and use here. https://codereview.chromium.org/1839503002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:130: // allocate memory during initialization (e.g. backtrace() does that). On 2016/04/01 15:56:28, Primiano Tucci wrote: > where is backtrace() called from? From StackTrace constructor. StackTrace is actually warmed up (there are special functions for that), but too late in case of startup tracing. https://codereview.chromium.org/1839503002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:179: std::reverse(ctx.backtrace.frames, On 2016/04/01 15:56:28, Primiano Tucci wrote: > I wonder if we could avoid this reverse and just walk backwards in > heap_profiler_stack_frame_deduplicator.cc when #ifdef BLA, to avoid this reverse > oepration on each malloc() (Could do in a separate CL, maybe just add a todo w/ > a crbug) OK https://codereview.chromium.org/1839503002/diff/40001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.cc (right): https://codereview.chromium.org/1839503002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:74: const StackFrame* begin = std::begin(backtrace.frames); On 2016/04/01 15:56:28, Primiano Tucci wrote: > if StackFrame is "const char*", I think > const char* const* begin > should become: > StackFrame const* begin > > same below Actually, 'const StackFrame*' and 'StackFrame const*' are equivalent here, and both produce 'void const* const*'. ('backtrace' is const&, so assigning 'backtrace.frames' to anything other than 'void const* const*' wouldn't compile.) https://codereview.chromium.org/1839503002/diff/40001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:239: if (size != 0) { On 2016/04/01 15:56:28, Primiano Tucci wrote: > uh do we see any zero allocations? Yup. Maybe we need to ignore them earlier, in MallocDumpProvider?
Description was changed from ========== Implement experimental allocation tracing. When enabled, stack trace for each allocation is recorded. Resulting trace file contains unsymbolized stack frames, and there is a script tools/trace/symbolize.py to symbolize them. Build with GN: use_experimental_allocation_tracing = true enable_profiling = true enable_nacl = false enable_full_stack_frames_for_profiling = true Run Chrome with --enable-heap-profiling flag. Trace with "memory-infra" category. BUG= ========== to ========== Implement experimental allocation tracing. When enabled, stack trace for each allocation is recorded. Resulting trace file contains unsymbolized stack frames, and there is a script tools/trace/symbolize.py to symbolize them. Build with GN: enable_profiling = true enable_full_stack_frames_for_profiling = true Run Chrome with --enable-heap-profiling=native option (!! notice =native part !!). Trace with "memory-infra" category. BUG= ==========
dskiba@google.com changed reviewers: + perezju@chromium.org
Ok this makes great sense to me. Thanks so much for doing this. I have only one major architectural doubt, that WalkStackFrames : I am a bit nervous about having to maintain yet another magic stack unwinding function. My question is: why can't we use just tcmalloc code? It's a matter of performances? Or just a matter of #include and DEPS. I will be happy to fix the problem if it's the latter. For the rest I have only minor comments, see below. https://codereview.chromium.org/1839503002/diff/60002/base/base_switches.cc File base/base_switches.cc (right): https://codereview.chromium.org/1839503002/diff/60002/base/base_switches.cc#n... base/base_switches.cc:23: // Report native (walk the stack) allocation traces. By default pseudo traces s/pseudo traces/pseudo stacks derived from trace events/ https://codereview.chromium.org/1839503002/diff/60002/base/base_switches.cc#n... base/base_switches.cc:25: const char kEnableHeapProfilingModeNative[] = "native"; Not sure if this constant (which is the value part for kEnableHeapProfiling should live here or where we use it. I'll leave this to base owners https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context.h (right): https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.h:38: // TODO: description :) https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.h:40: enum Type { enum class (thanks c++11), so we can get the right scoping and avoid TYPE_ prefix. https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.h:41: TYPE_SYMBOL, // const char* string Is this for the pseudo stack? In this case I think it should be TRACE_EVENT_NAME https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.h:43: TYPE_PC, // as returned by stack tracing (e.g. by StackTrace) Let's call it explicitly PROGRAM_COUNTER (or NATIVE_FRAME). Also I guess you meant "by stack unwinding" in the comment https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.h:66: bool BASE_EXPORT operator < (const StackFrame& lhs, const StackFrame& rhs); what do we need these operators for? https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.h:116: struct BASE_EXPORT hash<base::trace_event::StackFrame>: hash<const void*> { where do we hash StackFrame objects? Don't we keep them just in a flat array? https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:44: uintptr_t sp = reinterpret_cast<uintptr_t>( Should this have a #if !BUILDFLAG(ENABLE_PROFILING) || defined(NDEBUG) NOTREACHED() return 0; #endif to avoid end up using this when we shouldn't? https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:180: subtle::Acquire_Load(&capture_mode_)); We are going to call this on each malloc. I'd keep this a NoBarrier_Load and don't NOTREACHD() below if DONT_CAPTURE (just return an empty context) https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:190: auto src = pseudo_stack_.begin(); I think there is some duplicated logic between this and the next state. I think you could move up the auto src... auto dst block outside of the switch, and also do the if(thread_name_) check outside of the switch, and then just populated the dst pointers in the two cases https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context_tracker.h (right): https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.h:26: enum CaptureMode { enum class CaptureMode { DISABLED, PSEUDO_STACK, NATIVE_STACK } https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.h:33: // TODO: How to guards against CAPTURE_* -> DONT_CAPTURE -> CAPTURE_*? TODO want always a name ;) you can put mine if you want, as ruuda@ is unlikely to address that TODO you are removing. https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.h:37: inline static bool capture_enabled() { Hmm I think you want to expose the capture mode here. My rationale is: this is used by trace_log.cc to figure out if it should push inside the tracer or not. Right now with this CL you are letting it push the trace event in the tracker, but you are never reading those events back in the GetSnapshot(). Is it intended? Is it because you want to later support both native and pseudo stacks, and just didn't bother now? Or is this not intended and you just didn't think to that? https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.h:61: void PushPseudoStackFrame(const char* frame_symbol_value); s/frame_symbol_value/trace_event_name/ :) it makes clear what the arg is https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.h:62: void PushPseudoStackFrame(StackFrame frame); Do we really need both versions here? It seem that you still want the one that takes the char*. Also, if you take a StackFrame this is not necessarily "pseudo" anymore, as it could be native. https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.cc (right): https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:234: if (size != 0) { I think this has been fixed by kraynov@ in https://codereview.chromium.org/1859143003/ and should not happen anymore. https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/malloc... File base/trace_event/malloc_dump_provider.cc (right): https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/malloc... base/trace_event/malloc_dump_provider.cc:244: AllocationContext context = tracker->GetContextSnapshot(); why moving this? The point of this being before the lock is that you don't need to stall all the threads that are doing a malloc while you do the stack unwinding. If you are worried about hitting the NOTREACHED() in the NOT_ENABLED case, as I told above, remove the NOTREACHED() and let it happen. I'd not be too much obsessed about dealing with the corner cases on tracing enabled (as long as we don't return garbage) and prefer performances. https://codereview.chromium.org/1839503002/diff/60002/tools/trace/symbolize.py File tools/trace/symbolize.py (right): https://codereview.chromium.org/1839503002/diff/60002/tools/trace/symbolize.p... tools/trace/symbolize.py:1: #!/usr/bin/env python Did you figure out with perezju where to move this? Maybe Let's take it out of this CL to make it more reviawable?
Another thing came to my mind: wouldn't it be better to store relative PC addresses instead of absolute? I am thinking that, at trace dump time (when you seriualize the json) you could dump PC - address_of_a_known_function. That would avoid the need to look at mmaps in the py file (which will be a pain for Android because crazylinker relro packing shifts the load bias, so the mmaps will not give you enough information to figure out the right address in the .text section)
On 2016/04/07 15:57:01, Primiano Tucci wrote: > Another thing came to my mind: wouldn't it be better to store relative PC > addresses instead of absolute? > I am thinking that, at trace dump time (when you seriualize the json) you could > dump PC - address_of_a_known_function. > That would avoid the need to look at mmaps in the py file (which will be a pain > for Android because crazylinker relro packing shifts the load bias, so the mmaps > will not give you enough information to figure out the right address in the > .text section) We discussed this offline. We still need the full addresses if in future we want to support non-chrome libraries. THe plan for Android is to dump separately the address of a known function, so we can do the relative math. Let's keep that for another CL.
Description was changed from ========== Implement experimental allocation tracing. When enabled, stack trace for each allocation is recorded. Resulting trace file contains unsymbolized stack frames, and there is a script tools/trace/symbolize.py to symbolize them. Build with GN: enable_profiling = true enable_full_stack_frames_for_profiling = true Run Chrome with --enable-heap-profiling=native option (!! notice =native part !!). Trace with "memory-infra" category. BUG= ========== to ========== Implement experimental allocation tracing. When enabled, stack trace for each allocation is recorded. Resulting trace file contains unsymbolized stack frames, and there is a script tools/trace/symbolize.py to symbolize them. Build with GN: enable_profiling = true enable_full_stack_frames_for_profiling = true Run Chrome with --enable-heap-profiling=native option (!! notice =native part !!). Trace with "memory-infra" category. BUG=602701 ==========
https://codereview.chromium.org/1839503002/diff/60002/base/base_switches.cc File base/base_switches.cc (right): https://codereview.chromium.org/1839503002/diff/60002/base/base_switches.cc#n... base/base_switches.cc:23: // Report native (walk the stack) allocation traces. By default pseudo traces On 2016/04/07 15:51:56, Primiano Tucci wrote: > s/pseudo traces/pseudo stacks derived from trace events/ Done. https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context.h (right): https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.h:66: bool BASE_EXPORT operator < (const StackFrame& lhs, const StackFrame& rhs); On 2016/04/07 15:51:56, Primiano Tucci wrote: > what do we need these operators for? For inserting StackFrame in a map, deduplicator does that. https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context.h:116: struct BASE_EXPORT hash<base::trace_event::StackFrame>: hash<const void*> { On 2016/04/07 15:51:56, Primiano Tucci wrote: > where do we hash StackFrame objects? Don't we keep them just in a flat array? Just in case someone decides to put it into a hash map. Without hash specialization one might chose tree map just because operator< is available and he/she doesn't want to mess with implementing a hash. https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:180: subtle::Acquire_Load(&capture_mode_)); On 2016/04/07 15:51:56, Primiano Tucci wrote: > We are going to call this on each malloc. I'd keep this a NoBarrier_Load and > don't NOTREACHD() below if DONT_CAPTURE (just return an empty context) Done. https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:190: auto src = pseudo_stack_.begin(); On 2016/04/07 15:51:56, Primiano Tucci wrote: > I think there is some duplicated logic between this and the next state. > I think you could move up the auto src... auto dst block outside of the switch, > and also do the if(thread_name_) check outside of the switch, > and then just populated the dst pointers in the two cases Done. https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context_tracker.h (right): https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.h:26: enum CaptureMode { On 2016/04/07 15:51:56, Primiano Tucci wrote: > enum class CaptureMode { DISABLED, PSEUDO_STACK, NATIVE_STACK } Done. https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.h:33: // TODO: How to guards against CAPTURE_* -> DONT_CAPTURE -> CAPTURE_*? On 2016/04/07 15:51:56, Primiano Tucci wrote: > TODO want always a name ;) > you can put mine if you want, as ruuda@ is unlikely to address that TODO you are > removing. Done. https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.h:37: inline static bool capture_enabled() { On 2016/04/07 15:51:56, Primiano Tucci wrote: > Hmm I think you want to expose the capture mode here. > My rationale is: this is used by trace_log.cc to figure out if it should push > inside the tracer or not. > Right now with this CL you are letting it push the trace event in the tracker, > but you are never reading those events back in the GetSnapshot(). > Is it intended? Is it because you want to later support both native and pseudo > stacks, and just didn't bother now? > Or is this not intended and you just didn't think to that? Yes, this is intended. I want us to still maintain pseudo stack because we might want to use it as a filter for native traces, i.e. report native stack only if event X is on the pseudo stack. Basically I consider capturing mode an implementation detail, which might change, and outside world should care only about whether we are capturing or not. Besides, we might need to maintain pseudo stack for other components, like v8. https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.h:61: void PushPseudoStackFrame(const char* frame_symbol_value); On 2016/04/07 15:51:56, Primiano Tucci wrote: > s/frame_symbol_value/trace_event_name/ :) it makes clear what the arg is Done. https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.h:62: void PushPseudoStackFrame(StackFrame frame); On 2016/04/07 15:51:56, Primiano Tucci wrote: > Do we really need both versions here? > It seem that you still want the one that takes the char*. > Also, if you take a StackFrame this is not necessarily "pseudo" anymore, as it > could be native. Done. https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.cc (right): https://codereview.chromium.org/1839503002/diff/60002/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:234: if (size != 0) { On 2016/04/07 15:51:57, Primiano Tucci wrote: > I think this has been fixed by kraynov@ in > https://codereview.chromium.org/1859143003/ and should not happen anymore. Done.
ssid@chromium.org changed reviewers: + ssid@chromium.org
Not sure if i understand correctly, but shouldn't this cl be titled in the lines of "[tracing] Use native stack to track allocations in heap profiler"? experimental allocation tracing does not explain what experiment it is. We already have pseudo stack allocation tracing.
On 2016/04/13 22:00:26, ssid wrote: > Not sure if i understand correctly, but shouldn't this cl be titled in the > lines of "[tracing] Use native stack to track allocations in heap profiler"? > experimental allocation tracing does not explain what experiment it is. We > already have pseudo stack allocation tracing. I think this CL as it is is being split in pieces, right?
Description was changed from ========== Implement experimental allocation tracing. When enabled, stack trace for each allocation is recorded. Resulting trace file contains unsymbolized stack frames, and there is a script tools/trace/symbolize.py to symbolize them. Build with GN: enable_profiling = true enable_full_stack_frames_for_profiling = true Run Chrome with --enable-heap-profiling=native option (!! notice =native part !!). Trace with "memory-infra" category. BUG=602701 ========== to ========== [tracing] Add native allocation tracing mode. This change adds 'native' mode to '--enable-heap-profiling' option. When started with '--enable-heap-profiling=native' Chrome will keep native (real) stack traces for all live allocations. These native stack traces are then dumped to a trace file if 'memory-infra' category is enabled. Stack traces are dumped unsymbolized (for performance reasons), and there is a script (third_party/catapult/tracing/bin/symbolize_trace.py) that symbolizes all stack traces in a given trace file. Design document: https://goo.gl/DFoqfi Note: native stack tracing relies on frame pointers. You'll need to use either debug or a profiling release build (enable_profiling = true). Note: for now this all works on Linux @ x86/x64 only. BUG=602701 ==========
This review was rebased on top of two others: [tracing] Turn StackFrame into struct. https://codereview.chromium.org/1891543003/ Add function to trace stack using frame pointers. https://codereview.chromium.org/1879073002/ Now it contains only minimal changes needed. Please review.
dskiba@google.com changed reviewers: + mark@chromium.org
Mark, please review changes in base_switches.h/cc
//base/trace_event makes sense to me. Some comments, but overall LG. Will stamp once the dependent CL land and we get a ready to go version here. https://codereview.chromium.org/1839503002/diff/170001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1839503002/diff/170001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:19: static_cast<int>(AllocationContextTracker::CaptureMode::DISABLED); s/int/int32_t/ https://codereview.chromium.org/1839503002/diff/170001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:77: subtle::Release_Store(&capture_mode_, static_cast<int>(mode)); int32_t https://codereview.chromium.org/1839503002/diff/170001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:163: for (size_t i = 0; i != frame_count; ++i) { super slight optimization: if you make this a for(const void** frame = &frames[frame_count]; frame != &frames[0];) { --frame; *backtrace++ = *frame; } you avoid the frame_count - 1 - i math at each step probably there is even a c++11 way of doing this, just too late to think for me :) https://codereview.chromium.org/1839503002/diff/170001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.h (right): https://codereview.chromium.org/1839503002/diff/170001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:19: (BUILDFLAG(ENABLE_PROFILING) || defined(DEBUG)) Why do you need the (BUILDFLAG(ENABLE_PROFILING) || defined(DEBUG)) part here? Isn't the presence of FRAME_POINTERS enough? (And, ok, nacl for whatever reason I don't even want to know) https://codereview.chromium.org/1839503002/diff/170001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:35: enum class CaptureMode: int { s/int/int32_t/ https://codereview.chromium.org/1839503002/diff/170001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:48: inline static bool capture_enabled() { I think you want to return a CaptureMode here and change the code in trace_event.h to do: if (UNLIKELY(...capture_enabled())) to if (UNLIKELY(...capture_enabled() == PSEUDO_STACK)) otherwise the pseudo stack will unnecessarily being updated without any need. (unless you want both stack frames here, but your current code doesn't seem to support it, so let's not leave the codebase inconsistent) https://codereview.chromium.org/1839503002/diff/170001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1839503002/diff/170001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:188: LOG(ERROR) << "Ignored unknown mode '" << profiling_mode << "' for " there is a discussion on chromium-dev about LOG. TL;DR we shouldn't add any log statement. LOG(ERROR) is too easy to overlook. Let' make it hard crash. Let's make this as follows: First of all get rid of profiling_mode_parsed, doesn't seem really useful. if (profiling_mode == "") { ... } else if (profiling_mode == kNative) { #if ENABLE_NATIVE ... #else CHECK(false) << "Need either a debug or profiling=1 build to use bla=native" # } else { CHECK(false) << "Unsupported mode ..." }
LGTM in base, limited to base/base_switches.*. Please obtain LGTM from an OWNER of base/trace_event before committing this.
https://codereview.chromium.org/1839503002/diff/170001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1839503002/diff/170001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:19: static_cast<int>(AllocationContextTracker::CaptureMode::DISABLED); On 2016/04/19 20:15:19, Primiano Tucci wrote: > s/int/int32_t/ Done. https://codereview.chromium.org/1839503002/diff/170001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:77: subtle::Release_Store(&capture_mode_, static_cast<int>(mode)); On 2016/04/19 20:15:19, Primiano Tucci wrote: > int32_t Done. https://codereview.chromium.org/1839503002/diff/170001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:163: for (size_t i = 0; i != frame_count; ++i) { On 2016/04/19 20:15:19, Primiano Tucci wrote: > super slight optimization: > if you make this a > > for(const void** frame = &frames[frame_count]; frame != &frames[0];) { > --frame; > *backtrace++ = *frame; > } > > you avoid the frame_count - 1 - i math at each step > probably there is even a c++11 way of doing this, just too late to think for me > :) Done, but slightly differently. https://codereview.chromium.org/1839503002/diff/170001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.h (right): https://codereview.chromium.org/1839503002/diff/170001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:19: (BUILDFLAG(ENABLE_PROFILING) || defined(DEBUG)) On 2016/04/19 20:15:19, Primiano Tucci wrote: > Why do you need the (BUILDFLAG(ENABLE_PROFILING) || defined(DEBUG)) part here? > Isn't the presence of FRAME_POINTERS enough? (And, ok, nacl for whatever reason > I don't even want to know) Actually, you suggested these defines earlier :) But it makes sense - we can reliably use frame pointers only in debug and profiling builds, so for now this feature is limited to those configurations. https://codereview.chromium.org/1839503002/diff/170001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:35: enum class CaptureMode: int { On 2016/04/19 20:15:19, Primiano Tucci wrote: > s/int/int32_t/ Done. https://codereview.chromium.org/1839503002/diff/170001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:48: inline static bool capture_enabled() { On 2016/04/19 20:15:19, Primiano Tucci wrote: > I think you want to return a CaptureMode here and change the code in > trace_event.h to do: > if (UNLIKELY(...capture_enabled())) > to > if (UNLIKELY(...capture_enabled() == PSEUDO_STACK)) > > otherwise the pseudo stack will unnecessarily being updated without any need. > > (unless you want both stack frames here, but your current code doesn't seem to > support it, so let's not leave the codebase inconsistent) Done. https://codereview.chromium.org/1839503002/diff/170001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1839503002/diff/170001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:188: LOG(ERROR) << "Ignored unknown mode '" << profiling_mode << "' for " On 2016/04/19 20:15:19, Primiano Tucci wrote: > there is a discussion on chromium-dev about LOG. TL;DR we shouldn't add any log > statement. LOG(ERROR) is too easy to overlook. Let' make it hard crash. > Let's make this as follows: > First of all get rid of profiling_mode_parsed, doesn't seem really useful. > if (profiling_mode == "") { > ... > } else if (profiling_mode == kNative) { > #if ENABLE_NATIVE > ... > #else > CHECK(false) << "Need either a debug or profiling=1 build to use bla=native" > # > } else { > CHECK(false) << "Unsupported mode ..." > } Done.
LGTM with comment addressed. Please just have ssid cross-check your CL and LGTM it before landing. I reviewed 3 cls between you and ssid today and can't tell anymore what's supposed to be in ToT and what's supposed to be based on what. My head is in merge conflict state, I trust you guys will find a proper resolution to that while I am asleep. :) Thanks a lot for working on this https://codereview.chromium.org/1839503002/diff/170001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.h (right): https://codereview.chromium.org/1839503002/diff/170001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:19: (BUILDFLAG(ENABLE_PROFILING) || defined(DEBUG)) On 2016/04/20 19:19:25, Dmitry Skiba wrote: > On 2016/04/19 20:15:19, Primiano Tucci wrote: > > Why do you need the (BUILDFLAG(ENABLE_PROFILING) || defined(DEBUG)) part here? > > Isn't the presence of FRAME_POINTERS enough? (And, ok, nacl for whatever > reason > > I don't even want to know) > > Actually, you suggested these defines earlier :) But it makes sense - we can > reliably use frame pointers only in debug and profiling builds, so for now this > feature is limited to those configurations. oh right lolz :) https://codereview.chromium.org/1839503002/diff/190001/base/base_switches.cc File base/base_switches.cc (right): https://codereview.chromium.org/1839503002/diff/190001/base/base_switches.cc#... base/base_switches.cc:25: const char kEnableHeapProfilingModeNative[] = "native"; THougt for next CL: mariakhomenko@ is adding some heap-profiler config in the traceconfig. Maybe this native vs pseudo switch should also be there. Fine for now to not block this CL, but maybe we should think to that option really soon. https://codereview.chromium.org/1839503002/diff/190001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1839503002/diff/190001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:159: backtrace_end - backtrace, this will still return only the stack frames closest to malloc() not main right? if you have main() -> a() -> b() -> c() -> malloc(), and your backtrace size is 3, you will get here [a,b,c], not [main,a,b] right? Which seems the opposite of what the pseudo stack does. can we guarantee that we always saturate in the bottom, in other words that main() is always tehre and eventually lose the frames close to malloc(). In this way the clustering will always work, just lose details. (or probably I am just reading all this in reverse, in which case, ignore this , too late here) https://codereview.chromium.org/1839503002/diff/190001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:160: kKnownFrameCount); you can just s|kKnownFrameCount|1 /* skip_initial */| https://codereview.chromium.org/1839503002/diff/190001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.h (right): https://codereview.chromium.org/1839503002/diff/190001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:19: (BUILDFLAG(ENABLE_PROFILING) || defined(DEBUG)) I think that s/defined(DEBUG)/!defined(NDEBUG)/ is more reliable (40 vs 366 results in codesearch) https://codereview.chromium.org/1839503002/diff/190001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:48: inline static CaptureMode capture_mode() { make sure you sync-up with ssid's change, depending on who lands first :) https://codereview.chromium.org/1839503002/diff/190001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1839503002/diff/190001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:191: // If the above didn't crash, use some default uh wat? If the above didn't crash we have some serious problem in chrome and should stop shipping it :) CHECK is used to enforce security properties of chrome, if it doesn't work we are all screwed ant the heap profiler will be the last thing we should care :) plz just remove lines 190-193 https://codereview.chromium.org/1839503002/diff/190001/base/trace_event/trace... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1839503002/diff/190001/base/trace_event/trace... base/trace_event/trace_event.h:1059: using namespace base::trace_event; Just using base::trace_event::AllocationContextTracker; using namespace is forbidden by our coding style. ("You may not use a using-directive to make all names from a namespace available.") same below
https://codereview.chromium.org/1839503002/diff/190001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1839503002/diff/190001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:159: backtrace_end - backtrace, On 2016/04/21 20:08:21, Primiano Tucci wrote: > this will still return only the stack frames closest to malloc() not main right? > if you have main() -> a() -> b() -> c() -> malloc(), and your backtrace size is > 3, you will get here [a,b,c], not [main,a,b] right? > Which seems the opposite of what the pseudo stack does. > can we guarantee that we always saturate in the bottom, in other words that > main() is always tehre and eventually lose the frames close to malloc(). > In this way the clustering will always work, just lose details. > > (or probably I am just reading all this in reverse, in which case, ignore this , > too late here) No, you are right, I'm returning N bottom frames, and that is different from what pseudo stack returns. However, I think that this way it's more useful, because we can see who exactly allocating memory. With just top N frames, in most cases we won't see the full trace, and won't know exactly who is responsible. And I think that's the whole goal of native stack tracing. Because pseudo stack traces already give us high level overview. https://codereview.chromium.org/1839503002/diff/190001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1839503002/diff/190001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:191: // If the above didn't crash, use some default On 2016/04/21 20:08:21, Primiano Tucci wrote: > uh wat? > If the above didn't crash we have some serious problem in chrome and should stop > shipping it :) > CHECK is used to enforce security properties of chrome, if it doesn't work we > are all screwed ant the heap profiler will be the last thing we should care :) > plz just remove lines 190-193 Done. https://codereview.chromium.org/1839503002/diff/190001/base/trace_event/trace... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1839503002/diff/190001/base/trace_event/trace... base/trace_event/trace_event.h:1059: using namespace base::trace_event; On 2016/04/21 20:08:21, Primiano Tucci wrote: > Just using base::trace_event::AllocationContextTracker; > using namespace is forbidden by our coding style. > ("You may not use a using-directive to make all names from a namespace > available.") > > same below Done.
https://codereview.chromium.org/1839503002/diff/190001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1839503002/diff/190001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:159: backtrace_end - backtrace, On 2016/04/22 06:31:15, Dmitry Skiba wrote: > On 2016/04/21 20:08:21, Primiano Tucci wrote: > > this will still return only the stack frames closest to malloc() not main > right? > > if you have main() -> a() -> b() -> c() -> malloc(), and your backtrace size > is > > 3, you will get here [a,b,c], not [main,a,b] right? > > Which seems the opposite of what the pseudo stack does. > > can we guarantee that we always saturate in the bottom, in other words that > > main() is always tehre and eventually lose the frames close to malloc(). > > In this way the clustering will always work, just lose details. > > > > (or probably I am just reading all this in reverse, in which case, ignore this > , > > too late here) > > No, you are right, I'm returning N bottom frames, and that is different from > what pseudo stack returns. However, I think that this way it's more useful, > because we can see who exactly allocating memory. With just top N frames, in > most cases we won't see the full trace, and won't know exactly who is > responsible. And I think that's the whole goal of native stack tracing. Because > pseudo stack traces already give us high level overview. I understand this but I believe that all the clustering logic will be screwed from this, as it matches frames from top (main) to bottom (malloc). Which means now that when the heap profiler will tell you that main->RunLopo->RunTaskFoo used X mb of memory it will be a lie, because will count separately instances of RunTaskFoo(), as if they were a different entrypoint. Really, if the problem that you have is that 12 is too shallow as a depth just increase the depth to 24 or whatever. If we start returning lies is the end.
Description was changed from ========== [tracing] Add native allocation tracing mode. This change adds 'native' mode to '--enable-heap-profiling' option. When started with '--enable-heap-profiling=native' Chrome will keep native (real) stack traces for all live allocations. These native stack traces are then dumped to a trace file if 'memory-infra' category is enabled. Stack traces are dumped unsymbolized (for performance reasons), and there is a script (third_party/catapult/tracing/bin/symbolize_trace.py) that symbolizes all stack traces in a given trace file. Design document: https://goo.gl/DFoqfi Note: native stack tracing relies on frame pointers. You'll need to use either debug or a profiling release build (enable_profiling = true). Note: for now this all works on Linux @ x86/x64 only. BUG=602701 ========== to ========== [tracing] Add native allocation tracing mode. This change adds 'native' mode to '--enable-heap-profiling' option. When started with '--enable-heap-profiling=native' Chrome will keep native (real) stack traces for all live allocations. These native stack traces are then dumped to a trace file if 'memory-infra' category is enabled. Stack traces are dumped unsymbolized (for performance reasons), and there is a script (third_party/catapult/tracing/bin/symbolize_trace) that symbolizes all stack traces in a given trace file. Design document: https://goo.gl/DFoqfi Note: native stack tracing relies on frame pointers. You'll need to use either debug or a profiling release build (enable_profiling = true). Note: for now this all works on Linux @ x86/x64 only. BUG=602701 ==========
PS15 LGTM
The CQ bit was checked by dskiba@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org Link to the patchset: https://codereview.chromium.org/1839503002/#ps270001 (title: "Increase Backtrace frames, request even more frames")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839503002/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839503002/270001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by primiano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839503002/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839503002/270001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by dskiba@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839503002/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839503002/270001
Message was sent while issue was closed.
Description was changed from ========== [tracing] Add native allocation tracing mode. This change adds 'native' mode to '--enable-heap-profiling' option. When started with '--enable-heap-profiling=native' Chrome will keep native (real) stack traces for all live allocations. These native stack traces are then dumped to a trace file if 'memory-infra' category is enabled. Stack traces are dumped unsymbolized (for performance reasons), and there is a script (third_party/catapult/tracing/bin/symbolize_trace) that symbolizes all stack traces in a given trace file. Design document: https://goo.gl/DFoqfi Note: native stack tracing relies on frame pointers. You'll need to use either debug or a profiling release build (enable_profiling = true). Note: for now this all works on Linux @ x86/x64 only. BUG=602701 ========== to ========== [tracing] Add native allocation tracing mode. This change adds 'native' mode to '--enable-heap-profiling' option. When started with '--enable-heap-profiling=native' Chrome will keep native (real) stack traces for all live allocations. These native stack traces are then dumped to a trace file if 'memory-infra' category is enabled. Stack traces are dumped unsymbolized (for performance reasons), and there is a script (third_party/catapult/tracing/bin/symbolize_trace) that symbolizes all stack traces in a given trace file. Design document: https://goo.gl/DFoqfi Note: native stack tracing relies on frame pointers. You'll need to use either debug or a profiling release build (enable_profiling = true). Note: for now this all works on Linux @ x86/x64 only. BUG=602701 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:270001)
Message was sent while issue was closed.
Description was changed from ========== [tracing] Add native allocation tracing mode. This change adds 'native' mode to '--enable-heap-profiling' option. When started with '--enable-heap-profiling=native' Chrome will keep native (real) stack traces for all live allocations. These native stack traces are then dumped to a trace file if 'memory-infra' category is enabled. Stack traces are dumped unsymbolized (for performance reasons), and there is a script (third_party/catapult/tracing/bin/symbolize_trace) that symbolizes all stack traces in a given trace file. Design document: https://goo.gl/DFoqfi Note: native stack tracing relies on frame pointers. You'll need to use either debug or a profiling release build (enable_profiling = true). Note: for now this all works on Linux @ x86/x64 only. BUG=602701 ========== to ========== [tracing] Add native allocation tracing mode. This change adds 'native' mode to '--enable-heap-profiling' option. When started with '--enable-heap-profiling=native' Chrome will keep native (real) stack traces for all live allocations. These native stack traces are then dumped to a trace file if 'memory-infra' category is enabled. Stack traces are dumped unsymbolized (for performance reasons), and there is a script (third_party/catapult/tracing/bin/symbolize_trace) that symbolizes all stack traces in a given trace file. Design document: https://goo.gl/DFoqfi Note: native stack tracing relies on frame pointers. You'll need to use either debug or a profiling release build (enable_profiling = true). Note: for now this all works on Linux @ x86/x64 only. BUG=602701 Committed: https://crrev.com/5a05938adaabf413a40f1e0dfec3d1e4c683693a Cr-Commit-Position: refs/heads/master@{#389500} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/5a05938adaabf413a40f1e0dfec3d1e4c683693a Cr-Commit-Position: refs/heads/master@{#389500}
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:270001) has been created in https://codereview.chromium.org/1916033002/ by creis@chromium.org. The reason for reverting is: Appears to have caused a compile error on https://build.chromium.org/p/chromium/builders/Win%20x64/builds/92.. |
