Hook memory tracing into Windows heap shim.

Hook memory tracing into Windows heap shim.

Note for perf sheriffs
----------------------
This CL will cause a data stoppage on the "winheap" metric on
the memory benchmark. It is safe to ignore it as this CL is
renaming it to "malloc".

R=primiano@chromium.org
BUG=550886
Committed: https://crrev.com/7bec59a0644964141c9078d6f17307fdcbe628aa
Cr-Commit-Position: refs/heads/master@{#414523}

[Sigurður Ásgeirsson, 2016-08-23 20:00:09]

Hey Primiano,

I think it makes sense to hook the heap shim into tracing like this.
I've tested this and it seems to work, though the sums are always zero - I guess
walking the heap is the best way to acquire that info?
Alternatively, maybe it's best to (eventually) expose the stats collection on
the shim itself?
Are you the right person to review these changes?

Siggi

[Primiano Tucci (use gerrit), 2016-08-24 11:00:23]

Ah this make sense but we might want to fix up things a bit.
Let me explain some historical context.
Originally, before the heap profiler, we had:
malloc_dump_provider which was reporting the mallinfo() stats from malloc.
And at some point some window folk created winheap_dump_provider.cc that was
reporting winheap stats.

So far so good.
Then at some point we introduced the heap profiler and added that code to
malloc_dump_provider, purely because of convenience.

So the meaning of malloc_dump_provider became "dump the value of mallinfo() on
Linux/Mac BUT ALSO, if heap profiling is enabled, dump the heap profiler stats".
Which was a bit of a bend, but still okayish, because the heap profiler was
working only on Linux/Android, which is a subset of malloc_dump_provider.

Now we are in the state where heap profier works also on windows thanks to your
work. But now the two things don't intersect anymore, because the mallinfo()
stuff doesn't make sense on Windows but the heap profiler stuff does.

Having said this, I believe that the best thing to do at this point is:

just move the code of winheap_dump_provider into malloc_dump_provider. IIRC,
After all winheap is conceptually a specialization for malloc. Also at that
point we can call the column just "malloc" even on windows. And at that point
the heap profiler stuff will make sense.

Does it make sense?

https://codereview.chromium.org/2273703003/diff/20001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2273703003/diff/20001/base/BUILD.gn#newcode1195
base/BUILD.gn:1195: "trace_event/malloc_dump_provider.cc",
Uhm feels like malloc_dump_provider is now added to all platforms.
I suppose this can be just added up there around line 900? Maybe just 
in some if (!is_nacl)

[Sigurður Ásgeirsson, 2016-08-24 17:52:13]

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

[commit-bot: I haz the power, 2016-08-24 17:52:43]

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

[Sigurður Ásgeirsson, 2016-08-24 17:55:16]

Thanks - I relocated the relevant winheap code into the malloc_dump_provider as
you suggested. This makes sense to me.
I rejigged it a little bit to provide a "resident" and "virtual" estimate by
also counting uncommitted heap memory, which looks to be more consistent with
what happens on other platforms.
I can break that out to a separate patch if you like (the diff's not very
elucidating this way).
Please take another look?

https://codereview.chromium.org/2273703003/diff/20001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2273703003/diff/20001/base/BUILD.gn#newcode1195
base/BUILD.gn:1195: "trace_event/malloc_dump_provider.cc",
On 2016/08/24 11:00:23, Primiano Tucci wrote:
> Uhm feels like malloc_dump_provider is now added to all platforms.
> I suppose this can be just added up there around line 900? Maybe just 
> in some if (!is_nacl) 

Done.

[commit-bot: I haz the power, 2016-08-24 18:45:50]

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

[commit-bot: I haz the power, 2016-08-24 18:45:51]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Sigurður Ásgeirsson, 2016-08-24 20:40:22]

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

[commit-bot: I haz the power, 2016-08-24 20:41: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, 2016-08-24 23:45:27]

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

[commit-bot: I haz the power, 2016-08-24 23:45:28]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Primiano Tucci (use gerrit), 2016-08-25 16:49:07]

LGTM, thanks so much.

Can you just add something like this to the commit message:


Note for perf sheriffs
----------------------
This CL will cause a data stoppage on the "winheap" metric on the memory
benchmark. It is safe to ignore it as this CL is  renaming it to "malloc".

In absence of a bug you can use BUG=550886

https://codereview.chromium.org/2273703003/diff/60001/base/trace_event/malloc...
File base/trace_event/malloc_dump_provider.cc (right):

https://codereview.chromium.org/2273703003/diff/60001/base/trace_event/malloc...
base/trace_event/malloc_dump_provider.cc:152: for (size_t i = 0; i <
number_of_heaps; ++i) {
note for the future: I've no idea what these multiple heaps are (who creates
them? why there isn't just one) but if you feel is worth reporting them
individually, you can just create more entries and the UI will auto add them up.
In any case I wouldn't bother doing that in this CL.

https://codereview.chromium.org/2273703003/diff/60001/base/trace_event/malloc...
base/trace_event/malloc_dump_provider.cc:231: // Total allocated space is given
by |uordblks|.
nit add newline before this line.

[Sigurður Ásgeirsson, 2016-08-25 17:15:44]

Description was changed from

==========
Hook memory tracing into Windows heap shim.

R=primiano@chromium.org
BUG=
==========

to

==========
Hook memory tracing into Windows heap shim.

Note for perf sheriffs
----------------------
This CL will cause a data stoppage on the "winheap" metric on
the memory benchmark. It is safe to ignore it as this CL is
renaming it to "malloc".

R=primiano@chromium.org
BUG=550886
==========

[Sigurður Ásgeirsson, 2016-08-25 17:27:19]

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

[commit-bot: I haz the power, 2016-08-25 17:28:16]

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

[Sigurður Ásgeirsson, 2016-08-25 17:35:51]

siggi@chromium.org changed reviewers:
+ thakis@chromium.org

[Sigurður Ásgeirsson, 2016-08-25 17:35:52]

Thanks - Nico for base\BUILD.gn owners please?

https://codereview.chromium.org/2273703003/diff/60001/base/trace_event/malloc...
File base/trace_event/malloc_dump_provider.cc (right):

https://codereview.chromium.org/2273703003/diff/60001/base/trace_event/malloc...
base/trace_event/malloc_dump_provider.cc:152: for (size_t i = 0; i <
number_of_heaps; ++i) {
On 2016/08/25 16:49:07, Primiano Tucci wrote:
> note for the future: I've no idea what these multiple heaps are (who creates
> them? why there isn't just one) but if you feel is worth reporting them
> individually, you can just create more entries and the UI will auto add them
up.
> In any case I wouldn't bother doing that in this CL.
Added a lengthy comment at the top of the function.

I think this is fine as-is and it matches the current reporting, let's see
whether anyone complains.

To answer your question, the Windows heap API allows for an arbitrary number of
heaps and supplies operations to create and destroy them. Any binary (DLL or
EXE) that links statically against a copy of the CRT (this includes the CRT
itself), will historically create their own private heap. When such a binary is
unloaded, it'll delete its heap and thus scuttle all the crap it allocated.
To confuse things, there will be multiple versions of the CRT, and even
different binaries that dynamically link the CRT will load different ones, each
(historically) with its own heap.

https://codereview.chromium.org/2273703003/diff/60001/base/trace_event/malloc...
base/trace_event/malloc_dump_provider.cc:231: // Total allocated space is given
by |uordblks|.
On 2016/08/25 16:49:07, Primiano Tucci wrote:
> nit add newline before this line.

Done.

[Nico, 2016-08-25 17:50:58]

lgtm

[commit-bot: I haz the power, 2016-08-25 20:12:30]

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

[commit-bot: I haz the power, 2016-08-25 20:12:31]

Dry run: This issue passed the CQ dry run.

[Sigurður Ásgeirsson, 2016-08-25 20:15:32]

The CQ bit was checked by siggi@chromium.org

[Sigurður Ásgeirsson, 2016-08-25 20:15:32]

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/2273703003/#ps50008
(title: "Address Primiano's nits.")

[commit-bot: I haz the power, 2016-08-25 20:16:16]

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, 2016-08-25 20:22:42]

Description was changed from

==========
Hook memory tracing into Windows heap shim.

Note for perf sheriffs
----------------------
This CL will cause a data stoppage on the "winheap" metric on
the memory benchmark. It is safe to ignore it as this CL is
renaming it to "malloc".

R=primiano@chromium.org
BUG=550886
==========

to

==========
Hook memory tracing into Windows heap shim.

Note for perf sheriffs
----------------------
This CL will cause a data stoppage on the "winheap" metric on
the memory benchmark. It is safe to ignore it as this CL is
renaming it to "malloc".

R=primiano@chromium.org
BUG=550886
==========

[commit-bot: I haz the power, 2016-08-25 20:22:43]

Committed patchset #5 (id:50008)

[commit-bot: I haz the power, 2016-08-25 20:24:15]

Description was changed from

==========
Hook memory tracing into Windows heap shim.

Note for perf sheriffs
----------------------
This CL will cause a data stoppage on the "winheap" metric on
the memory benchmark. It is safe to ignore it as this CL is
renaming it to "malloc".

R=primiano@chromium.org
BUG=550886
==========

to

==========
Hook memory tracing into Windows heap shim.

Note for perf sheriffs
----------------------
This CL will cause a data stoppage on the "winheap" metric on
the memory benchmark. It is safe to ignore it as this CL is
renaming it to "malloc".

R=primiano@chromium.org
BUG=550886

Committed: https://crrev.com/7bec59a0644964141c9078d6f17307fdcbe628aa
Cr-Commit-Position: refs/heads/master@{#414523}
==========

[commit-bot: I haz the power, 2016-08-25 20:24:16]

Patchset 5 (id:??) landed as
https://crrev.com/7bec59a0644964141c9078d6f17307fdcbe628aa
Cr-Commit-Position: refs/heads/master@{#414523}