New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 686208 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug
Hotlist-MemoryInfra


Sign in to add a comment

Make virtual & native heap profiling work on Windows.

Project Member Reported by erikc...@chromium.org, Jan 27 2017

Issue description

I don't even know what the status is right now, so figuring that out would be the first step. It's possible this is already mostly done.
 
Cc: dskiba@chromium.org siggi@chromium.org
The pseudo-stack-mode should work, after siggi@ has ported the shim on Windows.
I've never tested it though, so would be grateful if somebody could check.

Note: kraynov@ is in the process of introducing a test to keep continuous track of it in  Issue 670828 

Instructions are here
https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/heap_profiler.md
TL;DR just start chrome with --enable-heap-profiling and use chrome://tracing ticking memory-infra


Instead I think some work is required for --enable-heap-profiling=native. I don't think we have a working stack unwinder plugged into. I am not even sure if we build with frame pointers. in lack of that, dskiba did a lot of heavy lifting for the stack scanner in //base/debug/stack_trace.cc , somebody might port that to win/x86_64.
Owner: ajwong@chromium.org
Status: Assigned (was: Available)
Blocking: 686292
Status: Started (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, Feb 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/772ff4cd3df488b794fa66e8ed9d2951543a11ee

commit 772ff4cd3df488b794fa66e8ed9d2951543a11ee
Author: wez <wez@chromium.org>
Date: Sat Feb 11 19:39:31 2017

Enable string pooling in the no_optimize configuration, under Windows.

This matches the default behaviour, even in Debug builds, of GCC & Clang.

AllocationContextTracker::PseudoStackFrame actually relies on string
pooling to allow it to compare trace event category and name strings by
address rather than strcmp().

BUG= 686208 

Review-Url: https://codereview.chromium.org/2689793003
Cr-Commit-Position: refs/heads/master@{#449874}

[modify] https://crrev.com/772ff4cd3df488b794fa66e8ed9d2951543a11ee/build/config/compiler/BUILD.gn

Cc: ssid@chromium.org
Hmm were you hitting 
https://cs.chromium.org/chromium/src/base/trace_event/heap_profiler_allocation_context_tracker.cc?rcl=2fd0fddb7497752f6e6abf8e3c7b996466cd62e9&l=130 ?
I think we should either remove that or make that a strcmp (depending on how much strcmp will slow down things in a non-release build).

Or is there any other place where we assume that identical strings have the same address?

We should NOT rely on the fact that strings are pooled, as that is in implementation detail (very likely to not hold in component builds, among the other things).
The only thing we should rely on is the fact that tracing strings are indefinitely lived, as that is an assumption that is everywhere in tracing.

Comment 7 by ajwong@chromium.org, Feb 13 2017

Agreed that the reliance on string pooling is a horrible idea. Turns out though that this code has been in production since 2015 so it felt better to just match windows match gcc in default behavior initially.

As for the component build breakage...also agreed, except that in this narrow case (wez vetted this first) our pooling expectations are actually bound to a single translation unit.

I think we're all on the same page but just to be explicit, the actual broken code is this whole operator== is semantically incorrect:

  https://bugs.chromium.org/p/chromium/issues/detail?id=686208#c6

I'm trying to understand the tracing macros a bit more before suggesting a course of action.

Comment 8 by ajwong@chromium.org, Feb 13 2017

ssid, primiano: you seem to be the most frequent touchers of this code... do you know if the TraceEvent::id_ field is the unique id concept we're looking for here?

If not, then I vote we kill this comparator and the DCHECK.
The original point of the dcheck is to spot cases where people use manual _begin and _end macro pairs and they are unbalanced (so they would cripple the pseudo stack tracking). Having said this the benefit is quite small and usually the stack should self sanitize once we go back to the message loop (% the unfortunate case of a_begin without a matching  _end). But sounds like this is causing troubles so removing the dcheck lgtm.

Re: tracing. I am pretty sure we don't rely (% accident) on the actual pointer. Tracing is all build around pointers to "category enabled words" and we should always use the pointer only to derive such pointers from strings via strcmp (see teacelog::getcatsegorygroupenabled). 
If you spot any instance of tracing relying on string pointer equality please file a bug and assign it to me, as that would be a bug. 
Thinking a bit more the right resolution to keep everybody happy should be 
DCHECK(a == b || strcmp(a, b) ==0)
That should still be fast in most cases and still work properly in the general case of lack of string pooling. 
I personally would prefer to remove the operator == since, it'd be nice to not allow people to feel like it's production ready code.

Having an embedded O(l) comparison for a set of objects that most people assume are extremely lightweight feels like bad hygiene.

For that matter, I think it'd be worth the effort to document the semantics of fields in TraceEvent. I can't figure out what most of the fields mean currently.

Comment 13 by w...@chromium.org, Feb 13 2017

FWIW running Chrome Canary on Windows, with --enable-heap-profiling, I'm able to get to 100% capture buffer used without crashing (yay ;). Browser process memory usage peaked at about 650-700MB while recording.

When I chose to stop recording, the browser stopped responding, and hung for 5-10 seconds while its memory usage ballooned to about 1.5GB before reducing back to about 700MB. Tracing failed with:

Error: Couldn't create an importer for the provided eventData.
    at Import.createImporter_ (chrome://tracing/tracing.js:1376:3418)
    at Import.createImports (chrome://tracing/tracing.js:1371:21)
    at Task.run (chrome://tracing/tracing.js:2198:13)
    at runAnother (chrome://tracing/tracing.js:2209:160)
    at runTask (chrome://tracing/tracing.js:2173:57)
    at processIdleWork (chrome://tracing/tracing.js:2179:116)
    at window.requestIdleCallback.timeout (chrome://tracing/tracing.js:2167:81)

Browser is still running, and browser process memory usage has dropped back down to "only" 140MB.
This isn't at all clear, but when you run with --enable-heap-profiling[=native], the tracing UI only really handles a very, very short trace. I generally stop after ~1 second. I think that if capture buffer hits 100%, the trace [even if it's not a memory trace] has a high probability of failing [hitting v8 memory limit of 2GB].

I don't know if this is a feature or bug of the tracing UI.

Comment 15 by ssid@chromium.org, Feb 14 2017

Re:6,10 Yes thinking about it we should remove the == comparison. Currently I don't think we can use strcmp in the place since it will cause too much overhead. I was just checking performance of profiling is too bad because of trace events alone.
I do not think we should be relying on the pointers being equal at any other point in the code. So, it should work fine in release builds.

Re:12 Having an O(l) comparison maybe is fine because we do this only when heap profiling is enabled. At that point the user has signed up for a slow session.

Re:13 Tracing UI is implemented in javascript and it loads the full json trace file at once. The maximum length of js string allowed is 256MiB in Chrome (this limit can be increased using some flags if really needed). As Eric pointed just 1 second trace is enough to get the snapshot of memory and it gives you all the details.
Usually running a normal trace with 100% buffer should not crash the UI since the trace file should be much smaller than 256MB, with average size of trace event. But this math doesn't work out very well for memory traces since memory dumps add the full snapshot of the process in a single trace event. So, the % shown in the buffer is not reliable.

As Primiano said, the pseudo stack profiling mode should already be working on Windows. Is this bug about how to get native stack traces on Windows? If so, why are we discussing about trace events? We do not enable trace events filtering in native profiling mode. In native mode we just get the stack frames using unwinding at each allocation. Maybe if we see other issues with pseudo stack profiling we should file different bugs and about it?
Re #15: FYI, the psuedo stack profiling was NOT working because this code caused a DCHECK in all renderer processes on start in windows debug mode. You're right that this should probably have been forked off onto another bug. Happy to do that if people feel strongly, but most of the discussion seems to be centralized here already.

I'll create a CL to shift the operator== to a more dangerously named function. As is, it's too easy reading code to assume that it's a fast operation.
First of all, let's not mix problems in the same bug. #13 is a more generic (And, i know, really bad) TraceViewer issue, independent of heap profiling, and is being tracked in https://github.com/catapult-project/catapult/issues/2826 . 
Now back to the original problem.

operator== vs "dangerously named function"
------------------------------------------
Style bike-shedding:
I don't fully buy the need of an explicit function. In C++ people are already used to the idea that operator == is not O(1). This is true today for std::string, base::StringPiece, base::FilePath etc. This case doesn't sound that any different.
Also, our C++ style guide says explicitly: "For example, prefer to define ==, =, and <<, rather than Equals(), CopyFrom(), and PrintTo()"

Practically:
There is a concrete reason why that operator== is there: those object end up in STL containers and operator== avoids the boilerplate of having to pass and define and explicit equals function. But this now brings to my next point: functional correctness.

Functional correctness: pointer vs deep string comparison
---------------------------------------------------------
Here's my high level position: we shouldn't rely on string pointer equality in a way that jeopardizes functional correctness (malfunctions / crashes / DCHECKs). That's a bug. Today we have a bug (thanks for filing this). However, we should speculate on that to improve our performances.
So, what I am saying is:
- from a design viewpoint it's okay if the heap profiler is faster when we have string pooling.
- we should speculate on the fact that most of the times we will have string pooling, because realistically that's true.
- we should make our code functionally correct in all cases. i.e. deal with the fact that in a minor number of cases identical strings won't be pooled. Today this is broken and this bug is about fixing this.
And, to clarify, the latter point is not just about #5 (MSVC build flags). There are so many subtle cases that can causes identical strings to not be pointer equal (component builds is one of them).

Enough talking, code plz
------------------------
Now, let's delve more in details and see where, today, we are making wrong assumptions that may break functional correctness.
After reading #11 and digging into the code, I found:
1) operator== in AllocationContextTracker::PseudoStackFrame (as reported by #11)
2) StackFrame (in heap_profiler_allocation_context.h) has operator==, != AND hash<base::trace_event::StackFrame> which all do rely on pure pointer identity.
   This makes perfect sense when |type|==PROGRAM_COUNTER but is broken when |type|==TRACE_EVENT_NAME or THREAD_NAME.
3) In turn 2. is used by Backtrace's operator== (Still in heap_profiler_allocation_context.h). Backtrace is essentially an array of StackFrame.
4) In turn 3 is also used by AllocationContext's operator== and hash operator, which does:
   return (lhs.backtrace == rhs.backtrace) && (lhs.type_name == rhs.type_name), which is broken on both sides: the 1st part of the equation is recursively broken becayse of 3. THe right side of the equation is broken because we make the same wrong assumption on the |type_name| (which is a const char*).

1 is seems used only by the aforementioned DCHECK and is easy to fix: either ditch the DCHECK (and hence the operator==) or use also strcmp (see my comment in #10).

4 is used by heap_profiler_heap_dump_writer.cc when producing the TracedValue that will be injected in the trace. this is NOT a fastpath and is also going to change soon: dskiba and hjd are changing and thankfully simplifying the heap dump format. Let's forget about this for a moment.

2 and 3 are used by the AllocationRegister's BacktraceMap |backtraces_| ( in heap_profiler_allocation_register.h) and are part of the fastpath: when we intercept an allocation we do two hashtable insertions: (i) we insert the address of the allocation (as returned by malloc) in a map<address, AllocationInfo> (this is NOT controversial as the allocation address is really unique); (ii) in order to avoid storing one full backtrace for each allocation, we deduplicate it using a the aforementioned BacktraceMap. Here's where the string identity issue comes into play.
These are tricky, what do we do here?

## Option 1
We deem two pseudo-StackFrame equal if their strings are deep-equal. This solution is: mentally clean, more complex code-wise, very likely perf-intrusive.
We can still make the operator== speculatively fast in most cases (look at pointer equality first, and than do deep strcmp). However I don't see any easy way around the hash operator. I think we have no options other than hashing the full contents of the string, and this might affect performances badly.

## Option 2
We bind the identity of stackrames (and hence BackTraces) to the string pointer and not the actual string content and reconcile everything in the HeapDumpWriter, which is out of the fastpath.
This solution is: mentally a bit more complex, clean and simple code-wise, non perf-intrusive, a bit more memory intrusive.

Won't Option 2 break things? Not if we factor this in into the HeapDumpWriter (the thing that writes the grouped snapshot into the trace)
So this is what I am saying here. Imagine that we have two allocations made by
"main" -> "MessageLoop::Run()" -> "TheFunctionName"(0x1234)
"main" -> "MessageLoop::Run()" -> "TheFunctionName"(0x5678)
(1234 and 5678 being the hypotetical const char* ptr).
Option 2 means that they will be treated as distinct entries by the heap profiler in the chromium C++ code, until the very last export stage.
Nothing is functionally wrong so far. Thankfully no lookup ever happens in the other direction (read: from within the heap profiler C++ code, we never query how many allocations are tied to "TheFunctionName", again % the exporter).
The only problem, so far, is that now we will waste two entries in the deduplicator because the two "TheFunctionName" have two identities. But again, assuming that string are *mostly* pooled this is not a real problem.

From a coding perspective this means leaving 2 and 3 as they are (% adding comments to explain what's going on).
The only place where the identity of "TheFunctionName" comes into play is at the exporter level, when we are in heap_profiler_heap_dump_writer.cc .
This is the thing that needs to change. This will have to group things by their actual content and hence doing full string comparison / hashing.
But now, here's the good news: this one is out of the fast path. This does NOT happen while we intercept a malloc/free call. This happens when we decide to inject a snapshot in the trace (today: every X seconds), and when it happens, it does run on a background thread. So making this even 2x slower doesn't make me worry too much.

So, all this has been a bit of a big braindump.
Trying to to make an overall balance, I would personally opt for fixing the DCHECK (in whatever form you like) and go for Option 2 here.
Rationale:
- It will keep everything as fast as possible.
- It requires no code changes (% comments) in the existing fastpath classes.
- It requires code changes in a piece of code (heap_profiler_heap_dump_writer.cc) that we were already planning to change, in order to improve the serialization format (dskiba, do we have a bug that tracks this work? if not we should, to keep everybody aligned).

Thoughts?

Comment 18 by w...@chromium.org, Feb 15 2017

Re == comparison: We can retain the DCHECK== if we add the TraceEvent::id() to PseudoStackFrame. WDYT?

Re Tracing UI / buffer sizes: I assumed that the trace %age referred to how much of some internal buffer was used; sounds like it's limited in the number of events it records, rather than their size?

Re pseudo vs native stack traces: This bug was originally about getting *any* heap profiling working under Windows, since it reliably crashed in Debug builds. We can now focus this bug on native heap profiling, yes. :)
> Re == comparison: We can retain the DCHECK== if we add the TraceEvent::id() to PseudoStackFrame. WDYT?
Unfortunately TraceEvent::id() being equal is the only case where we do not need the DCHECK. Let me expand more. There are mainly two types of trace events:
1) Scoped ones, based on RAII patterns -> TRACE_EVENTx
2) Manual pairs, based on manually putting TRACE_EVENT_BEGIN0 , TRACE_EVENT_END0

1 is really one event (so same id(), 2 are two distinct event objects with 2 different ids.
1 is never a problem and doesn't require the DCHECK at all, assuming that RAII works sanely (if it doesn't we should all go home)
2 is the real problem. We found that, from time to time, some code unbalances the BEGIN/END pairs. They are supposed to be used in a balanced pattern but there is nothing (other than this DCHECK) enforcing it.
The real problem is 2, and we cannot use id() there. 

> Re Tracing UI / buffer sizes: I assumed that the trace %age referred to how much of some internal buffer was used; sounds like it's limited in the number of events it records, rather than their size?
You are spot on. The problem of the current tracing design is that the buffer is fixed in terms of number of events (and hence the %age reflects that). Unfortunately at some point back in the history somebody decided that it was possible to give ownership of arbitrarily large objects to each event. And this creates the chaos: it's impossible to account and limit the amount of bytes that the trace buffer indirectly owns. For a more detailed description of the problem see https://bit.ly/TracingV2

> Re pseudo vs native stack traces: This bug was originally about getting *any* heap profiling working under Windows, since it reliably crashed in Debug builds. We can now focus this bug on native heap profiling, yes. :)
Fixing what described above will make both work. My only concern was just about not confusing crashes because of the DCHECK (which are in scope here) with crashes of the UI because the trace is too big (That's a more general tracing problem as discussed)
Blockedon: 694805 694792 694801 694810
As we keep dropping down this rabbit hole, it seems like we have more and more tasks.  Forking off this into sub-bugs now including moving the DCHECK discussion to 694810.
Project Member

Comment 21 by bugdroid1@chromium.org, Feb 24 2017

Blocking: 698266
Project Member

Comment 23 by bugdroid1@chromium.org, Mar 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/93de86905a40d330fd5418a772b7a4ef83491215

commit 93de86905a40d330fd5418a772b7a4ef83491215
Author: wez <wez@chromium.org>
Date: Tue Mar 14 18:44:07 2017

Make --enable-heap-profiling=native "work" on Windows.

Three small changes to get things "working"[1]:
1. Implement TraceStackFramePointers using StackTrace, under OS_WIN.
2. Add compile flag to enable frame pointers.
3. Tweak the HAVE_TRACE_STACK_FRAME_POINTERS definition.

This enables native heap profiling in Release+Static builds.

[1] Not perfectly, but enough to un-block related work.

BUG= 686208 

Review-Url: https://codereview.chromium.org/2707223002
Cr-Commit-Position: refs/heads/master@{#456771}

[modify] https://crrev.com/93de86905a40d330fd5418a772b7a4ef83491215/base/debug/stack_trace.cc
[modify] https://crrev.com/93de86905a40d330fd5418a772b7a4ef83491215/base/debug/stack_trace.h
[modify] https://crrev.com/93de86905a40d330fd5418a772b7a4ef83491215/base/debug/stack_trace_unittest.cc

Comment 24 by w...@chromium.org, Mar 20 2017

Blockedon: 703313
Project Member

Comment 25 by bugdroid1@chromium.org, Mar 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/22b017b9b241c47db8cf25c57c89c0b0d7fcfbe2

commit 22b017b9b241c47db8cf25c57c89c0b0d7fcfbe2
Author: wez <wez@chromium.org>
Date: Wed Mar 29 07:13:46 2017

Cleaner fall-back stack capture for --enable-heap-profiling=native.

This generalizes the fall-back to using base::debug::StackTrace to
capture stack traces in builds which lack frame pointers, allowing
native heap profiling to generate useful data, albeit with a more
significant performance penalty.

Changes made in the earlier patch for native heap profiling stack
capture under Windows are un-done in favour of the following:
1. MemoryDumpManager always allows native heap profiling[1].
2. HeapProfilerAllocationContextTracker chooses whether to use
   base::debug::StackTrace or TraceStackFramePointers() based on
   the value of HAVE_TRACE_STACK_FRAME_POINTERS().
3. HAVE_STACK_FRAME_POINTERS is no longer defined in configurations
   which lack frame pointers[2].

[1] Though note that only certain build configs actually support
    the necessary allocator shims; this will be addressed later.
[2] Frame pointers are typically only available in enable_profiling
    or Debug builds.

BUG= 686208 

Review-Url: https://codereview.chromium.org/2757123002
Cr-Commit-Position: refs/heads/master@{#460311}

[modify] https://crrev.com/22b017b9b241c47db8cf25c57c89c0b0d7fcfbe2/base/android/jni_android.cc
[modify] https://crrev.com/22b017b9b241c47db8cf25c57c89c0b0d7fcfbe2/base/debug/stack_trace.cc
[modify] https://crrev.com/22b017b9b241c47db8cf25c57c89c0b0d7fcfbe2/base/debug/stack_trace.h
[modify] https://crrev.com/22b017b9b241c47db8cf25c57c89c0b0d7fcfbe2/base/debug/stack_trace_unittest.cc
[modify] https://crrev.com/22b017b9b241c47db8cf25c57c89c0b0d7fcfbe2/base/trace_event/heap_profiler_allocation_context_tracker.cc
[modify] https://crrev.com/22b017b9b241c47db8cf25c57c89c0b0d7fcfbe2/base/trace_event/memory_dump_manager.cc

Project Member

Comment 26 by bugdroid1@chromium.org, Mar 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3731f7d26c0d826826453efc211d7902bc61e3d9

commit 3731f7d26c0d826826453efc211d7902bc61e3d9
Author: wez <wez@chromium.org>
Date: Thu Mar 30 06:22:51 2017

Revert of Cleaner fall-back stack capture for --enable-heap-profiling=native. (patchset #4 id:80001 of https://codereview.chromium.org/2757123002/ )

Reason for revert:
Broke Chrome for Android official builds (see crbug.com/706698), due to new dependency on debugging_flags.h header.

Original issue's description:
> Cleaner fall-back stack capture for --enable-heap-profiling=native.
>
> This generalizes the fall-back to using base::debug::StackTrace to
> capture stack traces in builds which lack frame pointers, allowing
> native heap profiling to generate useful data, albeit with a more
> significant performance penalty.
>
> Changes made in the earlier patch for native heap profiling stack
> capture under Windows are un-done in favour of the following:
> 1. MemoryDumpManager always allows native heap profiling[1].
> 2. HeapProfilerAllocationContextTracker chooses whether to use
>    base::debug::StackTrace or TraceStackFramePointers() based on
>    the value of HAVE_TRACE_STACK_FRAME_POINTERS().
> 3. HAVE_STACK_FRAME_POINTERS is no longer defined in configurations
>    which lack frame pointers[2].
>
> [1] Though note that only certain build configs actually support
>     the necessary allocator shims; this will be addressed later.
> [2] Frame pointers are typically only available in enable_profiling
>     or Debug builds.
>
> BUG= 686208 
>
> Review-Url: https://codereview.chromium.org/2757123002
> Cr-Commit-Position: refs/heads/master@{#460311}
> Committed: https://chromium.googlesource.com/chromium/src/+/22b017b9b241c47db8cf25c57c89c0b0d7fcfbe2

TBR=dskiba@chromium.org,primiano@chromium.org,dcheng@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 686208 

Review-Url: https://codereview.chromium.org/2790443002
Cr-Commit-Position: refs/heads/master@{#460679}

[modify] https://crrev.com/3731f7d26c0d826826453efc211d7902bc61e3d9/base/android/jni_android.cc
[modify] https://crrev.com/3731f7d26c0d826826453efc211d7902bc61e3d9/base/debug/stack_trace.cc
[modify] https://crrev.com/3731f7d26c0d826826453efc211d7902bc61e3d9/base/debug/stack_trace.h
[modify] https://crrev.com/3731f7d26c0d826826453efc211d7902bc61e3d9/base/debug/stack_trace_unittest.cc
[modify] https://crrev.com/3731f7d26c0d826826453efc211d7902bc61e3d9/base/trace_event/heap_profiler_allocation_context_tracker.cc
[modify] https://crrev.com/3731f7d26c0d826826453efc211d7902bc61e3d9/base/trace_event/memory_dump_manager.cc

Project Member

Comment 27 by bugdroid1@chromium.org, Apr 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2a567808f0a58c27d84845c8c178f33e79a1426b

commit 2a567808f0a58c27d84845c8c178f33e79a1426b
Author: erikchen <erikchen@chromium.org>
Date: Wed Apr 05 23:21:03 2017

Get rid of the base_paths target.

Fold the sources back into the base target. There's no good reason to have a
separate target to limit visiblity to :base.

BUG= 686208 ,706698

Review-Url: https://codereview.chromium.org/2797343002
Cr-Commit-Position: refs/heads/master@{#462265}

[modify] https://crrev.com/2a567808f0a58c27d84845c8c178f33e79a1426b/base/BUILD.gn

Blocking: 709526
Project Member

Comment 29 by bugdroid1@chromium.org, Apr 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/460b12442f72be8185a80cbf1ec0be6d11a27515

commit 460b12442f72be8185a80cbf1ec0be6d11a27515
Author: wez <wez@chromium.org>
Date: Mon Apr 10 21:55:36 2017

Cleaner fall-back stack capture for --enable-heap-profiling=native.

This generalizes the fall-back to using base::debug::StackTrace to
capture stack traces in builds which lack frame pointers, allowing
native heap profiling to generate useful data, albeit with a more
significant performance penalty.

Changes made in the earlier patch for native heap profiling stack
capture under Windows are un-done in favour of the following:
1. MemoryDumpManager always allows native heap profiling[1].
2. HeapProfilerAllocationContextTracker chooses whether to use
   base::debug::StackTrace or TraceStackFramePointers() based on
   the value of BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS).
3. BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS) is no longer defined in
   configurations which we cannot use frame pointers for unwinding[2].

[1] Though note that only certain build configs actually support
    the necessary allocator shims; this will be addressed later.
[2] Frame pointers are only available in profiling & Debug builds on
    some platforms, and are available but unsuitable for us to use for
    stack unwinding, on others.

BUG= 686208 

Review-Url: https://codereview.chromium.org/2757123002
Cr-Original-Commit-Position: refs/heads/master@{#460311}
Committed: https://chromium.googlesource.com/chromium/src/+/22b017b9b241c47db8cf25c57c89c0b0d7fcfbe2
Review-Url: https://codereview.chromium.org/2757123002
Cr-Commit-Position: refs/heads/master@{#463411}

[modify] https://crrev.com/460b12442f72be8185a80cbf1ec0be6d11a27515/base/debug/stack_trace.cc
[modify] https://crrev.com/460b12442f72be8185a80cbf1ec0be6d11a27515/base/debug/stack_trace.h
[modify] https://crrev.com/460b12442f72be8185a80cbf1ec0be6d11a27515/base/debug/stack_trace_unittest.cc
[modify] https://crrev.com/460b12442f72be8185a80cbf1ec0be6d11a27515/base/trace_event/heap_profiler_allocation_context_tracker.cc
[modify] https://crrev.com/460b12442f72be8185a80cbf1ec0be6d11a27515/base/trace_event/memory_dump_manager.cc
[modify] https://crrev.com/460b12442f72be8185a80cbf1ec0be6d11a27515/build/config/compiler/compiler.gni

Status: Fixed (was: Started)

Sign in to add a comment