[tracing] Don't register tracing-related allocations.

[tracing] Don't register tracing-related allocations.

Tracing-related scopes are instrumented with HEAP_PROFILER_SCOPED_IGNORE
macro, which caused all scoped allocations to have "tracing_overhead" type,
but didn't prevent those allocations to be registered.

However, it was discovered (see the bug) that the last stages of tracing
create substantial pressure on allocation register, adding millions of
short-lived allocations. Instead of bumping allocation register capacity
once again, this CL makes HEAP_PROFILER_SCOPED_IGNORE to completely ignore
allocations in its scope, i.e. not register them.

BUG=664350
Review-Url: https://codereview.chromium.org/2637783002
Cr-Commit-Position: refs/heads/master@{#444500}
Committed: https://chromium.googlesource.com/chromium/src/+/9ab14b2d78c1d15909fa2d6cc12760a757edb405

[DmitrySkiba, 2017-01-16 04:48:59]

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

[Primiano Tucci (use gerrit), 2017-01-16 10:18:39]

On 2017/01/16 04:48:59, DmitrySkiba wrote:

Hmm I see what you are doing here, however the problem here is that you are:
1) going to make the heap profiler output more cryptic. The reason why we chose
to report "tracing_overhead" back then was to make sure that the total matches
(To the best of what we know inside of chrome)
2) adding branches to the heap profiler fast paths.

I took a look to the bug, is the crash affecting only android? Honestly my
proposal would be:
On android: bump up the kAllocationCapacity to 5M as well to match desktop. I
think we have been just too conservative when doing the choice in
crbug.com/617772
If I did the math right, on 32-bit sizeof(Cell) == 12. so even with 5M it should
be 5M * 12 = ~60M of usage. Note that it would be 60 M of virtual memory, not
necessarily dirty. The heap profiler has a freelist internally and we don't bump
up the watermark unless we really need.
This would keep things simple and clean, at the cost of some extra memory.
Unless there is another practical reason why we can't just bump the size Android
(Are we hitting virtual address space limits?).

On desktop (Actually, on 64 bit, which today translates into mostly desktop): 
while looking at this issue I just realized that we have some good margin of
improvements there. Today our cell is:
 struct AllocationInfo {
    size_t size;
    const char* type_name;
    BacktraceMap::KVIndex backtrace_index;  (KVIndex is a size_t)
  };
which is 24 bytes. However if you think about that, both |size| and the
|backtrace_index| could just be uint32_t. We are not going to serve allocations
> 4 GB (the security layers would murder chrome if somebody tries that), neither
we are going to keep > 4 Billion backtraces.
So my proposal is to make:
using... KVIndex = uint32_t
 struct AllocationInfo {
    uint32_t size;
    BacktraceMap::KVIndex backtrace_index;  (KVIndex is a size_t)
    const char* type_name;
  };

(Note the reordering of fields to get packing). This would bring
sizeof(AllocationInfo), which is the hashtable cell from 24 -> 16 bytes, saving
40 MB of memory with zero loss. if you really want you can use this to bump up
further the HT size to 7M on desktop: the total virtual memory used would be
unchanged and the dirty memory would be less because of smaller structs &
packing.
WDYT?

[DmitrySkiba, 2017-01-17 17:46:10]

On 2017/01/16 10:18:39, Primiano Tucci wrote:
> On 2017/01/16 04:48:59, DmitrySkiba wrote:
> 
> Hmm I see what you are doing here, however the problem here is that you are:
> 1) going to make the heap profiler output more cryptic. The reason why we
chose
> to report "tracing_overhead" back then was to make sure that the total matches
> (To the best of what we know inside of chrome)
> 2) adding branches to the heap profiler fast paths.
> 
> I took a look to the bug, is the crash affecting only android? Honestly my
> proposal would be:
> On android: bump up the kAllocationCapacity to 5M as well to match desktop. I
> think we have been just too conservative when doing the choice in
> crbug.com/617772
> If I did the math right, on 32-bit sizeof(Cell) == 12. so even with 5M it
should
> be 5M * 12 = ~60M of usage. Note that it would be 60 M of virtual memory, not
> necessarily dirty. The heap profiler has a freelist internally and we don't
bump
> up the watermark unless we really need.
> This would keep things simple and clean, at the cost of some extra memory.
> Unless there is another practical reason why we can't just bump the size
Android
> (Are we hitting virtual address space limits?).
> 
> On desktop (Actually, on 64 bit, which today translates into mostly desktop): 
> while looking at this issue I just realized that we have some good margin of
> improvements there. Today our cell is:
>  struct AllocationInfo {
>     size_t size;
>     const char* type_name;
>     BacktraceMap::KVIndex backtrace_index;  (KVIndex is a size_t)
>   };
> which is 24 bytes. However if you think about that, both |size| and the
> |backtrace_index| could just be uint32_t. We are not going to serve
allocations
> > 4 GB (the security layers would murder chrome if somebody tries that),
neither
> we are going to keep > 4 Billion backtraces.
> So my proposal is to make:
> using... KVIndex = uint32_t
>  struct AllocationInfo {
>     uint32_t size;
>     BacktraceMap::KVIndex backtrace_index;  (KVIndex is a size_t)
>     const char* type_name;
>   };
> 
> (Note the reordering of fields to get packing). This would bring
> sizeof(AllocationInfo), which is the hashtable cell from 24 -> 16 bytes,
saving
> 40 MB of memory with zero loss. if you really want you can use this to bump up
> further the HT size to 7M on desktop: the total virtual memory used would be
> unchanged and the dirty memory would be less because of smaller structs &
> packing.
> WDYT?

I like the 64-bit compaction idea!

There are two things that bother me in the JSON conversion:

1. It produces 2.3M *simultaneously* allocated objects in the process. I.e. it
creates huge pressure on the register.
2. Most of those objects are then deallocated. I.e. it just recycles lots of
memory.

And all that is to get tracing_overhead number that then gets in the way while
analyzing heap profiles.

Yes, we could bump the capacity, but that would just mask two real problems:

1. Inefficiency of JSON conversion.
2. Uselessness of tracing_overhead number.

If we want tracing_overhead number, we can probably estimate it after the
conversion (and add to a proper place, not to the malloc tree).

[Primiano Tucci (use gerrit), 2017-01-17 17:54:09]

alright, LGTM as per offline/bug discussion.
thanks

https://codereview.chromium.org/2637783002/diff/1/base/trace_event/heap_profi...
File base/trace_event/heap_profiler_allocation_context_tracker.cc (right):

https://codereview.chromium.org/2637783002/diff/1/base/trace_event/heap_profi...
base/trace_event/heap_profiler_allocation_context_tracker.cc:160: if
(ignore_scope_depth_) {
drop the braces at this point

[Primiano Tucci (use gerrit), 2017-01-17 17:57:49]

On 2017/01/17 17:46:10, DmitrySkiba wrote:
> Yes, we could bump the capacity, but that would just mask two real problems:
> 
> 1. Inefficiency of JSON conversion.

yes this is because JsonWrier requires to write a tree of base::Value which is
all heap based.
We could write a smarter one but there are too many more important things we
need to solve first.

[DmitrySkiba, 2017-01-17 21:38:48]

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

[DmitrySkiba, 2017-01-17 21:38:49]

haraken@: please review changes in WebKit (GetContextSnapshot() API changed).

[haraken, 2017-01-17 23:47:49]

LGTM

[DmitrySkiba, 2017-01-17 23:56:08]

The CQ bit was checked by dskiba@chromium.org

[DmitrySkiba, 2017-01-17 23:56:09]

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/2637783002/#ps20001
(title: "Address comments")

[commit-bot: I haz the power, 2017-01-17 23:56:41]

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-01-18 00:05:27]

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

[commit-bot: I haz the power, 2017-01-18 00:05:27]

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...)

[DmitrySkiba, 2017-01-18 19:27:18]

The CQ bit was checked by dskiba@chromium.org

[DmitrySkiba, 2017-01-18 19:27:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2637783002/#ps40001
(title: "Fix unittest")

[commit-bot: I haz the power, 2017-01-18 19:28:07]

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-01-18 21:53:47]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484767638379510,
"parent_rev": "48660c60ba986114ad4e6a4acb69e940202f14e5", "commit_rev":
"9ab14b2d78c1d15909fa2d6cc12760a757edb405"}

[commit-bot: I haz the power, 2017-01-18 21:54:23]

Description was changed from

==========
[tracing] Don't register tracing-related allocations.

Tracing-related scopes are instrumented with HEAP_PROFILER_SCOPED_IGNORE
macro, which caused all scoped allocations to have "tracing_overhead" type,
but didn't prevent those allocations to be registered.

However, it was discovered (see the bug) that the last stages of tracing
create substantial pressure on allocation register, adding millions of
short-lived allocations. Instead of bumping allocation register capacity
once again, this CL makes HEAP_PROFILER_SCOPED_IGNORE to completely ignore
allocations in its scope, i.e. not register them.

BUG=664350
==========

to

==========
[tracing] Don't register tracing-related allocations.

Tracing-related scopes are instrumented with HEAP_PROFILER_SCOPED_IGNORE
macro, which caused all scoped allocations to have "tracing_overhead" type,
but didn't prevent those allocations to be registered.

However, it was discovered (see the bug) that the last stages of tracing
create substantial pressure on allocation register, adding millions of
short-lived allocations. Instead of bumping allocation register capacity
once again, this CL makes HEAP_PROFILER_SCOPED_IGNORE to completely ignore
allocations in its scope, i.e. not register them.

BUG=664350

Review-Url: https://codereview.chromium.org/2637783002
Cr-Commit-Position: refs/heads/master@{#444500}
Committed:
https://chromium.googlesource.com/chromium/src/+/9ab14b2d78c1d15909fa2d6cc127...
==========

[commit-bot: I haz the power, 2017-01-18 21:54:29]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/9ab14b2d78c1d15909fa2d6cc127...