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

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}
Committed: https://chromium.googlesource.com/chromium/src/+/460b12442f72be8185a80cbf1ec0be6d11a27515

[Wez, 2017-03-19 03:23:21]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-19 03:23:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-19 03:52:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-19 03:52:23]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Wez, 2017-03-21 23:06:02]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 23:06:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 23:13:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 23:13:13]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Wez, 2017-03-22 00:01:16]

wez@chromium.org changed reviewers:
+ dskiba@chromium.org

[Wez, 2017-03-22 00:01:17]

PTAL - cleanup of my earlier patch to make --enable-heap-profiling=native more
widely available (even if it is still Too Damn Slow to use, in most builds).

[Primiano Tucci (use gerrit), 2017-03-22 10:38:34]

primiano@chromium.org changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2017-03-22 10:38:35]

defer the actual changes in base/debug to dskiba, who wrote the linux/android
part/ and base owners.
Thanks a lot for making this happen //base/trace_event/ LGTM

https://codereview.chromium.org/2757123002/diff/40001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2757123002/diff/40001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:191: } else if (profiling_mode ==
switches::kEnableHeapProfilingModeNative) {
should we have an #if defined(COMPONENT_BUILD) here to just die with a "don't
even try, we can't support component builds, too much trouble sorry."?
Not sure what is the state of component build on various platforms. If it works
I am happy. But if it doesn't (or is just too painful to maintain) I am very
happy if we just give a clear signal saying "nope, static build only".

I'd just avoid mixed signals, where we pretend that it works, but then at the
end it doesn't. finding that after some spending some hours of attempts can be
quite frustrating for people.

[DmitrySkiba, 2017-03-23 19:10:24]

I have several questions:

1. Why HAVE_STACK_FRAME_POINTERS was converted into a function-style macro? I
modeled it after other HAVE_* macros (e.g. HAVE_SYMBOLIZE (Chrome-specific), or
HAVE_MMAP, HAVE_CONFIG_H, etc.).

2. Why HAVE_STACK_FRAME_POINTERS depends on BUILDFLAG(ENABLE_PROFILING)? The
macro is supposed to tell you about whether TraceStackFramePointers() is
available (i.e. supports current compiler / arch / bitness combination), and not
whether unwinding a stack would succeed (it can fail for many different reasons,
e.g. unwinding from a 3rd party library built without frame pointers).

Also, some platforms are always built with frame pointers, for example Android
64-bit ARM.

Another thing is that last time I tried to use BUILDFLAG(ENABLE_PROFILING) in a
base/ header, I ended up in misery, because debugging_flags.h is generated when
base/ is built, but there is nothing preventing ninja from starting building
both foo/ (which depend on base/) and base/ at the same time. If something in
foo/ includes stack_trace.h and depends on HAVE_STACK_FRAME_POINTERS we end up
with non-deterministic build.


But I like other part of the CL, namely changes in
heap_profiler_allocation_context_tracker.cc and memory_dump_manager.cc.

https://codereview.chromium.org/2757123002/diff/40001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2757123002/diff/40001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:191: } else if (profiling_mode ==
switches::kEnableHeapProfilingModeNative) {
On 2017/03/22 10:38:35, Primiano Tucci wrote:
> should we have an #if defined(COMPONENT_BUILD) here to just die with a "don't
> even try, we can't support component builds, too much trouble sorry."?
> Not sure what is the state of component build on various platforms. If it
works
> I am happy. But if it doesn't (or is just too painful to maintain) I am very
> happy if we just give a clear signal saying "nope, static build only".
> 
> I'd just avoid mixed signals, where we pretend that it works, but then at the
> end it doesn't. finding that after some spending some hours of attempts can be
> quite frustrating for people.

Component builds *should* be working on Linux and Android, not sure about other
platforms. I'll double check.

[Wez, 2017-03-27 03:52:19]

On 2017/03/23 19:10:24, DmitrySkiba wrote:
> I have several questions:
> 
> 1. Why HAVE_STACK_FRAME_POINTERS was converted into a function-style macro? I
> modeled it after other HAVE_* macros (e.g. HAVE_SYMBOLIZE (Chrome-specific),
or
> HAVE_MMAP, HAVE_CONFIG_H, etc.).

As discussed offline: Rationale is to make it hard to find that the macro is not
set
simply because you forgot to include the header. Have changed this back, for
consistency
with the other HAVE_ macros though, as per our discussion on moving the macro
definition
to be driven by GN-side logic.

> 2. Why HAVE_STACK_FRAME_POINTERS depends on BUILDFLAG(ENABLE_PROFILING)? The
> macro is supposed to tell you about whether TraceStackFramePointers() is
> available (i.e. supports current compiler / arch / bitness combination), and
not
> whether unwinding a stack would succeed (it can fail for many different
reasons,
> e.g. unwinding from a 3rd party library built without frame pointers).

As discussed offline: HAVE_TRACE_STACK_FRAME_POINTERS expresses whether the
TraceStackFramePointers() function makes sense in the current build; for most
configurations it does not unless ENABLE_PROFILING is set. Issues tracing
through
third-party code are independent of that.

> Also, some platforms are always built with frame pointers, for example Android
> 64-bit ARM.

Fair point; noted that in crbug.com/699863.

[Wez, 2017-03-27 03:58:09]

Patchset #4 (id:60001) has been deleted

[Wez, 2017-03-27 03:59:37]

https://codereview.chromium.org/2757123002/diff/40001/base/debug/stack_trace.h
File base/debug/stack_trace.h (right):

https://codereview.chromium.org/2757123002/diff/40001/base/debug/stack_trace....
base/debug/stack_trace.h:34: #define HAVE_TRACE_STACK_FRAME_POINTERS() 1
FYI: Turned this back into a non-function-style macro.

https://codereview.chromium.org/2757123002/diff/40001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2757123002/diff/40001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:191: } else if (profiling_mode ==
switches::kEnableHeapProfilingModeNative) {
On 2017/03/22 10:38:35, Primiano Tucci wrote:
> should we have an #if defined(COMPONENT_BUILD) here to just die with a "don't
> even try, we can't support component builds, too much trouble sorry."?
> Not sure what is the state of component build on various platforms. If it
works
> I am happy. But if it doesn't (or is just too painful to maintain) I am very
> happy if we just give a clear signal saying "nope, static build only".

That would equally apply to the pseudo-stack case, above, since its a question
of whether we have working allocator shims, I believe.

Rather than an explicit check for component build here, I'd prefer to test a
macro indicating whether or not we have working allocator shims, and have that
set alongside the shimming code (or in the GN if necessary) appropriately to the
build type. 

> I'd just avoid mixed signals, where we pretend that it works, but then at the
> end it doesn't. finding that after some spending some hours of attempts can be
> quite frustrating for people.

Agreed - ideally I'd prefer that we pop up a helpful message as well, rather
than just CHECK()ing to generate a stack trace!

https://codereview.chromium.org/2757123002/diff/40001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:191: } else if (profiling_mode ==
switches::kEnableHeapProfilingModeNative) {
On 2017/03/23 19:10:24, DmitrySkiba wrote:
> On 2017/03/22 10:38:35, Primiano Tucci wrote:
> > should we have an #if defined(COMPONENT_BUILD) here to just die with a
"don't
> > even try, we can't support component builds, too much trouble sorry."?
> > Not sure what is the state of component build on various platforms. If it
> works
> > I am happy. But if it doesn't (or is just too painful to maintain) I am very
> > happy if we just give a clear signal saying "nope, static build only".
> > 
> > I'd just avoid mixed signals, where we pretend that it works, but then at
the
> > end it doesn't. finding that after some spending some hours of attempts can
be
> > quite frustrating for people.
> 
> Component builds *should* be working on Linux and Android, not sure about
other
> platforms. I'll double check. 

Definitely not on Windows, since the shim impl is relies on statically-linked
MSVCRT.

[Wez, 2017-03-27 03:59:47]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 04:00:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 05:13:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 05:13:59]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2017-03-27 09:42:47]

https://codereview.chromium.org/2757123002/diff/40001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2757123002/diff/40001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:191: } else if (profiling_mode ==
switches::kEnableHeapProfilingModeNative) {
On 2017/03/27 03:59:37, Wez wrote:
> On 2017/03/22 10:38:35, Primiano Tucci wrote:
> > should we have an #if defined(COMPONENT_BUILD) here to just die with a
"don't
> > even try, we can't support component builds, too much trouble sorry."?
> > Not sure what is the state of component build on various platforms. If it
> works
> > I am happy. But if it doesn't (or is just too painful to maintain) I am very
> > happy if we just give a clear signal saying "nope, static build only".
> 
> That would equally apply to the pseudo-stack case, above, since its a question
> of whether we have working allocator shims, I believe.

Ah, I was thinking more to the "can this unwinder cope with multiple .so(s)".
but you make another good point.

> Rather than an explicit check for component build here, I'd prefer to test a
> macro indicating whether or not we have working allocator shims, and have that
> set alongside the shimming code (or in the GN if necessary) appropriately to
the
> build type. 

+1 I like this. To be clear doesn't  have to happen in this CL, but I'd just
kindly ask that this happens at some point in the near future.
Filed crbug.com/705461 to keep track of this.

> > I'd just avoid mixed signals, where we pretend that it works, but then at
the
> > end it doesn't. finding that after some spending some hours of attempts can
be
> > quite frustrating for people.
> 
> Agreed - ideally I'd prefer that we pop up a helpful message as well, rather
> than just CHECK()ing to generate a stack trace!

[Wez, 2017-03-28 02:53:54]

The CQ bit was checked by wez@chromium.org

[Wez, 2017-03-28 02:53:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/2757123002/#ps80001
(title: "Revert to non-function-style macro")

[Wez, 2017-03-28 02:54:15]

The CQ bit was unchecked by wez@chromium.org

[commit-bot: I haz the power, 2017-03-28 02:54:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Wez, 2017-03-28 05:19:52]

wez@chromium.org changed reviewers:
+ dcheng@chromium.org

[Wez, 2017-03-28 05:19:53]

dcheng: Could you provide OWNERS for base/[debug]/, plz?

[dcheng, 2017-03-28 06:16:23]

LGTM with two questions

https://codereview.chromium.org/2757123002/diff/80001/base/trace_event/heap_p...
File base/trace_event/heap_profiler_allocation_context_tracker.cc (right):

https://codereview.chromium.org/2757123002/diff/80001/base/trace_event/heap_p...
base/trace_event/heap_profiler_allocation_context_tracker.cc:219: const void*
const* frames = stack_trace.Addresses(&frame_count);
Nit: this will capture the constructor for StackTrace and this function as well?
Is it worth trimming to match the HAVE_STACK_FRAME_POINTERS path?

(Also, the CL description says that HAVE_STACK_FRAME_POINTERS is now a
function-style macro, but it's not invoked with () here?)

[Wez, 2017-03-28 06:20:34]

Description was changed from

==========
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 now a function-style macro, and 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
==========

to

==========
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
==========

[Wez, 2017-03-28 06:38:22]

https://codereview.chromium.org/2757123002/diff/80001/base/trace_event/heap_p...
File base/trace_event/heap_profiler_allocation_context_tracker.cc (right):

https://codereview.chromium.org/2757123002/diff/80001/base/trace_event/heap_p...
base/trace_event/heap_profiler_allocation_context_tracker.cc:219: const void*
const* frames = stack_trace.Addresses(&frame_count);
On 2017/03/28 06:16:23, dcheng wrote:
> Nit: this will capture the constructor for StackTrace and this function as
well?
> Is it worth trimming to match the HAVE_STACK_FRAME_POINTERS path?

That is a fair point; it doesn't affect the functionality per-se but reducing
the number of pointless frames to symbolize SGTM. :)

I think this call-site should trim a single frame, and StackTrace should
probably itself be fixed to trim the right number of frames - I think that's
tricky to determine, though, in optimized builds.

> (Also, the CL description says that HAVE_STACK_FRAME_POINTERS is now a
> function-style macro, but it's not invoked with () here?)

CL used to do that but dskiba@ preferred to stay consistent with the other HAVE_
macros, so reverted that change - have fixed description.

[Wez, 2017-03-29 04:55:09]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2017-03-29 04:55:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 07:13:52]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490763309296340,
"parent_rev": "19d5f8b3d7cd8ba33ba3f2dd331c339384b299dd", "commit_rev":
"22b017b9b241c47db8cf25c57c89c0b0d7fcfbe2"}

[commit-bot: I haz the power, 2017-03-29 07:14:37]

Description was changed from

==========
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
==========

to

==========
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/+/22b017b9b241c47db8cf25c57c89...
==========

[commit-bot: I haz the power, 2017-03-29 07:14:40]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/22b017b9b241c47db8cf25c57c89...

[Wez, 2017-03-30 06:21:48]

A revert of this CL (patchset #4 id:80001) has been created in
https://codereview.chromium.org/2790443002/ by wez@chromium.org.

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

[Wez, 2017-03-30 06:22:27]

Description was changed from

==========
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/+/22b017b9b241c47db8cf25c57c89...
==========

to

==========
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/+/22b017b9b241c47db8cf25c57c89...
==========

[Wez, 2017-04-05 22:14:56]

On 2017/03/30 06:21:48, Wez wrote:
> A revert of this CL (patchset #4 id:80001) has been created in
> https://codereview.chromium.org/2790443002/ by mailto:wez@chromium.org.
> 
> The reason for reverting is: Broke Chrome for Android official builds (see
> crbug.com/706698), due to new dependency on debugging_flags.h header..

Build failure was due to //base having a :base_paths sub-component, which failed
to specify any actual dependencies, instead using the
allow_cyclical_dependencies hack to pass GN check - this is being fixed in
https://codereview.chromium.org/2797343002/ after which this CL can re-land
as-is.

[Wez, 2017-04-06 00:16:01]

Patchset #5 (id:100001) has been deleted

[Wez, 2017-04-06 00:24:19]

The CQ bit was checked by wez@chromium.org

[Wez, 2017-04-06 00:24:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2757123002/#ps120001
(title: "Rebase")

[Wez, 2017-04-06 00:24:21]

The CQ bit was unchecked by wez@chromium.org

[Wez, 2017-04-06 00:24:26]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 00:25:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 01:05:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 01:05:22]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Wez, 2017-04-06 01:20:42]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 01:21:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 02:45:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 02:45:07]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Wez, 2017-04-06 17:25:37]

The CQ bit was checked by wez@chromium.org

[Wez, 2017-04-06 17:25:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2757123002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-06 17:26:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Wez, 2017-04-06 17:26:47]

Description was changed from

==========
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/+/22b017b9b241c47db8cf25c57c89...
==========

to

==========
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_TRACE_STACK_FRAME_POINTERS is no longer defined in
   configurations which we know 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, though some platforms enable them by default.

BUG=686208

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

[commit-bot: I haz the power, 2017-04-06 21:23:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 21:23:42]

Failed to apply patch for base/android/jni_android.cc:
While running git apply --index -3 -p1;
  error: patch failed: base/android/jni_android.cc:29
  Falling back to three-way merge...
  Applied patch to 'base/android/jni_android.cc' with conflicts.
  U base/android/jni_android.cc

Patch:       base/android/jni_android.cc
Index: base/android/jni_android.cc
diff --git a/base/android/jni_android.cc b/base/android/jni_android.cc
index
56dc5c2ff3fe22c2e4628075beaf1097f60e2f91..960be3af8017a43a4847bae67f1d5bebd192aa17
100644
--- a/base/android/jni_android.cc
+++ b/base/android/jni_android.cc
@@ -11,7 +11,6 @@
 #include "base/android/build_info.h"
 #include "base/android/jni_string.h"
 #include "base/android/jni_utils.h"
-#include "base/debug/debugging_flags.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/threading/thread_local.h"
@@ -29,7 +28,7 @@
base::LazyInstance<base::android::ScopedJavaGlobalRef<jobject>>::Leaky
     g_class_loader = LAZY_INSTANCE_INITIALIZER;
 jmethodID g_class_loader_load_class_method_id = 0;
 
-#if BUILDFLAG(ENABLE_PROFILING) && HAVE_TRACE_STACK_FRAME_POINTERS
+#if HAVE_TRACE_STACK_FRAME_POINTERS
 base::LazyInstance<base::ThreadLocalPointer<void>>::Leaky
     g_stack_frame_pointer = LAZY_INSTANCE_INITIALIZER;
 #endif
@@ -289,7 +288,7 @@ std::string GetJavaExceptionInfo(JNIEnv* env, jthrowable
java_throwable) {
   return ConvertJavaStringToUTF8(exception_string);
 }
 
-#if BUILDFLAG(ENABLE_PROFILING) && HAVE_TRACE_STACK_FRAME_POINTERS
+#if HAVE_TRACE_STACK_FRAME_POINTERS
 
 JNIStackFrameSaver::JNIStackFrameSaver(void* current_fp) {
   previous_fp_ = g_stack_frame_pointer.Pointer()->Get();
@@ -304,7 +303,7 @@ void* JNIStackFrameSaver::SavedFrame() {
   return g_stack_frame_pointer.Pointer()->Get();
 }
 
-#endif  // ENABLE_PROFILING && HAVE_TRACE_STACK_FRAME_POINTERS
+#endif  // HAVE_TRACE_STACK_FRAME_POINTERS
 
 }  // namespace android
 }  // namespace base

[Wez, 2017-04-07 01:20:50]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 01:21:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Wez, 2017-04-07 01:22:07]

dcheng: FYI this CL seems to confuse git cl format with the change in
heap_profiler_allocation_context_tracker, due to preprocessor macros.  Am I
holding it wrong?

[Wez, 2017-04-07 01:25:55]

wez@chromium.org changed reviewers:
+ brettw@chromium.org, erikchen@chromium.org

[Wez, 2017-04-07 01:25:56]

erikchen: PTAL at the migration of the OS_WIN checks into the .gn.
brettw: OWNERS for compiler.gni, plz.

[erikchen, 2017-04-07 01:47:43]

Nice clean up, thanks. commit message is out of date. otherwise lgtm.

https://codereview.chromium.org/2757123002/diff/160001/base/trace_event/heap_...
File base/trace_event/heap_profiler_allocation_context_tracker.cc (right):

https://codereview.chromium.org/2757123002/diff/160001/base/trace_event/heap_...
base/trace_event/heap_profiler_allocation_context_tracker.cc:232: #endif  //
!defined(OS_NACL)
hm. Any reason you moved this #endif?

[commit-bot: I haz the power, 2017-04-07 03:04:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 03:04:47]

Dry run: This issue passed the CQ dry run.

[Wez, 2017-04-07 20:24:08]

Description was changed from

==========
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_TRACE_STACK_FRAME_POINTERS is no longer defined in
   configurations which we know 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, though some platforms enable them by default.

BUG=686208

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

to

==========
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-Commit-Position: refs/heads/master@{#460311}
Committed:
https://chromium.googlesource.com/chromium/src/+/22b017b9b241c47db8cf25c57c89...
==========

[Wez, 2017-04-08 00:11:32]

FYI

https://codereview.chromium.org/2757123002/diff/160001/base/trace_event/heap_...
File base/trace_event/heap_profiler_allocation_context_tracker.cc (right):

https://codereview.chromium.org/2757123002/diff/160001/base/trace_event/heap_...
base/trace_event/heap_profiler_allocation_context_tracker.cc:232: #endif  //
!defined(OS_NACL)
On 2017/04/07 01:47:43, erikchen wrote:
> hm. Any reason you moved this #endif?

Moving the #endif means not having to have a dummy |frames| and |frame_count|
definition, just to have this loop compile, when the loop itself will be a no-op
(due to |frame_count| being zero in the NaCl case).

Since this case means that NATIVE_STACK doesn't work under NaCl, I'm
re-introducing a preprocessor condition around the command-line option.

[Wez, 2017-04-08 00:11:42]

The CQ bit was checked by wez@chromium.org

[Wez, 2017-04-08 00:11:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, dcheng@chromium.org, erikchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2757123002/#ps180001
(title: "Disable native heap profiling option under NaCl")

[commit-bot: I haz the power, 2017-04-08 00:12:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-08 00:21:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-08 00:21:44]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[brettw, 2017-04-10 18:29:35]

lgtm

https://codereview.chromium.org/2757123002/diff/180001/build/config/compiler/...
File build/config/compiler/compiler.gni (right):

https://codereview.chromium.org/2757123002/diff/180001/build/config/compiler/...
build/config/compiler/compiler.gni:95: # Windows 32-bit does provide frame
pointers, but the compiler does not provide
80 cols?

[Wez, 2017-04-10 18:49:10]

https://codereview.chromium.org/2757123002/diff/180001/build/config/compiler/...
File build/config/compiler/compiler.gni (right):

https://codereview.chromium.org/2757123002/diff/180001/build/config/compiler/...
build/config/compiler/compiler.gni:95: # Windows 32-bit does provide frame
pointers, but the compiler does not provide
On 2017/04/10 18:29:35, brettw (plz ping after 24h) wrote:
> 80 cols?

Seriously, gn format, you only had to do the one thing... ;)

Done.

[Wez, 2017-04-10 18:49:56]

The CQ bit was checked by wez@chromium.org

[Wez, 2017-04-10 18:49:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, erikchen@chromium.org, brettw@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2757123002/#ps200001
(title: "Fix comments")

[commit-bot: I haz the power, 2017-04-10 18:50:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 21:55:42]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1491850196845700,
"parent_rev": "28f186338efaf147e981787624c2ca223b6efe40", "commit_rev":
"460b12442f72be8185a80cbf1ec0be6d11a27515"}

[commit-bot: I haz the power, 2017-04-10 21:56:33]

Description was changed from

==========
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-Commit-Position: refs/heads/master@{#460311}
Committed:
https://chromium.googlesource.com/chromium/src/+/22b017b9b241c47db8cf25c57c89...
==========

to

==========
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/+/22b017b9b241c47db8cf25c57c89...
Review-Url: https://codereview.chromium.org/2757123002
Cr-Commit-Position: refs/heads/master@{#463411}
Committed:
https://chromium.googlesource.com/chromium/src/+/460b12442f72be8185a80cbf1ec0...
==========

[commit-bot: I haz the power, 2017-04-10 21:56:35]

Committed patchset #9 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/460b12442f72be8185a80cbf1ec0...