[Tentaive patch for discussion] Add Purge+Suspend metrics as UMA.

[Tentaive patch for discussion] Add Purge+Suspend metrics as UMA.

Discussion point:
- Do we need two UMA for each allocator, i.e. Shrink and Growth?
- [partition alloc] is it better to use totalActiveBytes or totalResidentBytes?
- [blinkGC] is it better to use totalBytes or totalAllocatedBytes?

BUG=635419
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq

[tasak, 2016-09-20 11:49:47]

The CQ bit was checked by tasak@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 11:50:04]

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

[tasak, 2016-09-20 11:50:33]

tasak@google.com changed reviewers:
+ bashi@chromium.org, haraken@chromium.org, primiano@chromium.org

[commit-bot: I haz the power, 2016-09-20 11:57:59]

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

[commit-bot: I haz the power, 2016-09-20 11:58:00]

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

[haraken, 2016-09-20 12:19:14]

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

[haraken, 2016-09-20 12:19:15]

+isherman for a UMA question. See render_thread_impl.cc.

For Blink GC, I'd use totalAllocatedObjectSize + totalMarkedObjectSize.

For PartitionAlloc, I'd use residentBytes.

Note that what we want to count is the value reported in the "size" field in
memory-infra.

https://codereview.chromium.org/2350423003/diff/1/content/renderer/render_thr...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2350423003/diff/1/content/renderer/render_thr...
content/renderer/render_thread_impl.cc:1817:
UMA_HISTOGRAM_MEMORY_KB("PurgeAndSuspend.TotalGrowth", -total_diff);

isherman@: Is there any way to report a negative number to UMA?

https://codereview.chromium.org/2350423003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebMemoryStatistics.cpp (right):

https://codereview.chromium.org/2350423003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebMemoryStatistics.cpp:50:
statistics.blinkGCTotalAllocatedBytes = ProcessHeap::totalMarkedObjectSize() +
ProcessHeap::totalMarkedObjectSize();

Why are you adding ProcessHeap::totalMarkedObjectSize() twice?

[tasak, 2016-09-21 06:33:19]

Description was changed from

==========
[Tentaive patch for discussion] Add Purge+Suspend metrics as UMA.

Discussion point:
- Do we need two UMA for each allocator, i.e. Shrink and Growth?
- [partition alloc] is it better to use totalActiveBytes or totalResidentBytes?
- [blinkGC] is it better to use totalBytes or totalAllocatedBytes?

BUG=635419
==========

to

==========
[Tentaive patch for discussion] Add Purge+Suspend metrics as UMA.

Discussion point:
- Do we need two UMA for each allocator, i.e. Shrink and Growth?
- [partition alloc] is it better to use totalActiveBytes or totalResidentBytes?
- [blinkGC] is it better to use totalBytes or totalAllocatedBytes?

BUG=635419
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
==========

[tasak, 2016-09-21 06:43:31]

Patchset #2 (id:20001) has been deleted

[tasak, 2016-09-21 06:49:51]

The CQ bit was checked by tasak@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 06:50:13]

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

[tasak, 2016-09-21 06:50:18]

Patchset #2 (id:40001) has been deleted

[tasak, 2016-09-21 06:50:43]

https://codereview.chromium.org/2350423003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebMemoryStatistics.cpp (right):

https://codereview.chromium.org/2350423003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebMemoryStatistics.cpp:50:
statistics.blinkGCTotalAllocatedBytes = ProcessHeap::totalMarkedObjectSize() +
ProcessHeap::totalMarkedObjectSize();
On 2016/09/20 12:19:15, haraken wrote:
> 
> Why are you adding ProcessHeap::totalMarkedObjectSize() twice?

Done.
Sorry. I copied the code from BlinkGCMemoryDumpProvider.

[commit-bot: I haz the power, 2016-09-21 07:00:24]

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

[commit-bot: I haz the power, 2016-09-21 07:00:25]

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

[haraken, 2016-09-21 07:37:49]

Overall looks good. We might want to get a metric of V8's memory on the main
thread though.

primiano@ should take a look once he's back.

[Ilya Sherman, 2016-09-21 21:15:48]

https://codereview.chromium.org/2350423003/diff/1/content/renderer/render_thr...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2350423003/diff/1/content/renderer/render_thr...
content/renderer/render_thread_impl.cc:1817:
UMA_HISTOGRAM_MEMORY_KB("PurgeAndSuspend.TotalGrowth", -total_diff);
On 2016/09/20 12:19:14, haraken wrote:
> 
> isherman@: Is there any way to report a negative number to UMA?

Yes, using a sparse histogram.  But, it's not very convenient for something like
what you're trying to do, where you want logarithmically scaled buckets.  So, I
don't really have a better suggestion than what you've implemented here, unless
you want to manually map from values to their corresponding bucket number --
sorry.

[bashi, 2016-09-22 23:28:26]

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

https://codereview.chromium.org/2350423003/diff/60001/base/trace_event/malloc...
base/trace_event/malloc_dump_provider.h:52: static MallocStatistics
GetStatistics();
I'd propose to have a separate base/memory/malloc_statistics{.h,.cc} and move
GetStatistics() into MallocStatistics. Then we no longer need to depend on
memory_infra :)

https://codereview.chromium.org/2350423003/diff/60001/content/child/child_dis...
File content/child/child_discardable_shared_memory_manager.cc (right):

https://codereview.chromium.org/2350423003/diff/60001/content/child/child_dis...
content/child/child_discardable_shared_memory_manager.cc:238: size_t
ChildDiscardableSharedMemoryManager::GetMemoryUsage() const {
It may be consistent when this returns a struct like malloc?

class ChildDiscardableSharedMemoryManager {
  ...
  struct Statistics {
    size_t total_size;
    size_t freelist_size;
  };
};

https://codereview.chromium.org/2350423003/diff/60001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2350423003/diff/60001/content/renderer/render...
content/renderer/render_thread_impl.cc:1763: base::TimeDelta::FromSeconds(5));
(just for note; I understand this is WIP)

Rather than using a timer, I'd prefer to have a callback which is invoked when
all purging tasks are done.

https://codereview.chromium.org/2350423003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebMemoryStatistics.cpp (right):

https://codereview.chromium.org/2350423003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebMemoryStatistics.cpp:40: WebMemoryStatistics
WebMemoryStatistics::Get()
(just for note; I understand this is WIP)

We might want to ask esprehn@ whether Source/web is the right place to do this.

[tasak, 2016-09-23 08:03:56]

The CQ bit was checked by tasak@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 08:04:18]

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-09-23 08:42:02]

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

[commit-bot: I haz the power, 2016-09-23 08:42:03]

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

[Primiano Tucci (use gerrit), 2016-09-23 09:24:34]

Hmm honestly it looks to me this patch is adding a lot of churn here and there
to the codebase, gets only data from some few places and just to avoid doing the
right thing (that is, what I explained in this thread go/yolky). For instance is
missing things like: what is the total effect on the resident size of the
process?

I'd honestly like to try and have a clean solution here. You are probably not
the last one who will want to have some uma metric like this. And once people
add more dependencies to all these hack this code will be impossible to rip out.

Should we do something more planned (make this an actual OKR), have a discussion
with UMA folks to see if there is a way to cluster all these histograms and
whatnot?

https://codereview.chromium.org/2350423003/diff/80001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2350423003/diff/80001/content/renderer/render...
content/renderer/render_thread_impl.cc:1764: base::TimeDelta::FromSeconds(5));
This smells odd. I understand purging is not sync. Doesn't it provide a "Done"
async callback though?

[tasak, 2016-09-23 10:12:18]

Thank you, Primiano.

On 2016/09/23 09:24:34, Primiano Tucci wrote:
> Hmm honestly it looks to me this patch is adding a lot of churn here and there
> to the codebase, gets only data from some few places and just to avoid doing
the
> right thing (that is, what I explained in this thread go/yolky). For instance
is
> missing things like: what is the total effect on the resident size of the
> process?
> 
> I'd honestly like to try and have a clean solution here. You are probably not
> the last one who will want to have some uma metric like this. And once people
> add more dependencies to all these hack this code will be impossible to rip
out.
> 
> Should we do something more planned (make this an actual OKR), have a
discussion
> with UMA folks to see if there is a way to cluster all these histograms and
> whatnot?

I agree with you. We should do this...

So is it possible to start purge+suspend experiment and the task in parallel?

I mean, I will add comment like "Add TODO(tasak): This code is just for
purge+suspend experiment, and should be cleaned up..."
and will start purge+suspend. (so we will get some data)
In parallel, I will add this clean-up task to the team OKR, write some document
about what I want to do, and discuss with UMA folks.

I would like to start purge+suspend experiment as soon as possible.

https://codereview.chromium.org/2350423003/diff/80001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2350423003/diff/80001/content/renderer/render...
content/renderer/render_thread_impl.cc:1764: base::TimeDelta::FromSeconds(5));
On 2016/09/23 09:24:34, Primiano Tucci wrote:
> This smells odd. I understand purging is not sync. Doesn't it provide a "Done"
> async callback though?

I agree with you. When we have MemoryCoordinatorV0, I will add "Done" callback.

Talking about current MemoryPressureListener... I have a patch which modifies
observer_list_threadsafe.h to add "NotifyAndReply" method.
However, I think, the patch is too much...

[Primiano Tucci (use gerrit), 2016-09-23 19:31:09]

> So is it possible to start purge+suspend experiment and the task in parallel?
Well at this point I wonder: can we just use the existing memory UMA (which
tracks resident memory) as a first data point for purge+suspend? and get the
other metrics once we make it work?
That would make this patch super simple and avoid pulling in hacky code.

https://codereview.chromium.org/2350423003/diff/80001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2350423003/diff/80001/content/renderer/render...
content/renderer/render_thread_impl.cc:1764: base::TimeDelta::FromSeconds(5));
On 2016/09/23 10:12:18, tasak wrote:
> On 2016/09/23 09:24:34, Primiano Tucci wrote:
> > This smells odd. I understand purging is not sync. Doesn't it provide a
"Done"
> > async callback though?
> 
> I agree with you. When we have MemoryCoordinatorV0, I will add "Done"
callback.
> 
> Talking about current MemoryPressureListener... I have a patch which modifies
> observer_list_threadsafe.h to add "NotifyAndReply" method.
> However, I think, the patch is too much...

Ok I see. ack. maybe add a comment here.

[tasak, 2016-09-26 05:34:11]

>Well at this point I wonder: can we just use the existing memory UMA (which
tracks resident memory) as a first data point for purge+suspend? and get the
other metrics once we make it work?

I would like to confirm. Your suggestion is that it is better to re-use the
exising UMAs: Memory.Renderer.Large2, ... and so on for purge+suspend?

(1) Because the UMAs (and ProcessData used to generate the UMAs) are only
available for browser process.

Since the UMAs are reported in chrome/browser/metrics/metrics_memory_details.cc,
only browser process can report the UMAs, I think.
However purge+suspend is implemented in renderer processes.
So if we want to re-use the UMAs, we probably need to send some IPC message from
renderer process to browser process.
I'm not sure whether the messaging code is simple or not.

(2) Because in the near future I would like to replace the hacky code with
memory-infra.

I expect that it will be possible to invoke memory-infra without tracing in the
near future. Since my hacky code is based on OnMemoryDump, I expect that we can
compare the hacky purge+suspend UMAs with UMAs using memory-infra.

(3) Because I would like to see each allocator's memory usage if possible.

If we have the data, at least it will be possible to see whether "purge" is
working well or not.
For example, I have already found that sometimes "malloc" memory usage increases
because it is not possible to suspend compositor-related workers.

[tasak, 2016-09-26 06:47:02]

The CQ bit was checked by tasak@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 06:47:13]

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-09-26 07:27:04]

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

[commit-bot: I haz the power, 2016-09-26 07:27:05]

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

[tasak, 2016-09-26 08:11:43]

The CQ bit was checked by tasak@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 08:12:09]

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-09-26 09:19:45]

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

[commit-bot: I haz the power, 2016-09-26 09:19:46]

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-09-26 18:34:57]

On Mon, Sep 26, 2016 at 6:34 AM, tasak via tracing <tracing@chromium.org>
wrote:

> I would like to confirm. Your suggestion is that it is better to re-use the
> exising UMAs: Memory.Renderer.Large2, ... and so on for purge+suspend?
>
Well, the code that they call. You definitely need to create some new
histograms to check before/after.
What I am saying here is that instead of rushing up something that will
give: (i) harder maintenance (ii) more codereview traffic (iii) an
intermediate solution (you'll get only few of the memory-infra columns
anyways), let's start with a v0 which gives you something simple and clean
(the current UMA histograms), and let's shoot for doing the right thing in
v1.


> (1) Because the UMAs (and ProcessData used to generate the UMAs) are only
> available for browser process.
>
> Since the UMAs are reported in chrome/browser/metrics/
> metrics_memory_details.cc,
> only browser process can report the UMAs, I think.
> However purge+suspend is implemented in renderer processes.
> So if we want to re-use the UMAs, we probably need to send some IPC
> message from
> renderer process to browser process.
> I'm not sure whether the messaging code is simple or not.
>

If I read the code correctly, all that stuff just uses
base::ProcessMetrics::GetWorkingSetKBytesStatm at the end.
Can you use the same in your UMA from the renderer?


> (2) Because in the near future I would like to replace the hacky code with
> memory-infra.
>
> I expect that it will be possible to invoke memory-infra without tracing
> in the
> near future.
>
Yes, that's the plan.


> Since my hacky code is based on OnMemoryDump, I expect that we can
> compare the hacky purge+suspend UMAs with UMAs using memory-infra.
>
Yeah but that hacky OnMemoryDump will very likely interfere with things
like the existing slow-reports and general use of tracing. If it starts
randomly crashing perf benchmarks on telemetry, or people trying to use
chrome://tracing it will be a pain to debug those races.



> (3) Because I would like to see each allocator's memory usage if possible.
>
Can you add just a call to mallinfo()? That will be a temporary but
"self-contained" solution, which will not affect other pieces of the
codebase.


> If we have the data, at least it will be possible to see whether "purge" is
> working well or not.
> For example, I have already found that sometimes "malloc" memory usage
> increases
> because it is not possible to suspend compositor-related workers.
>
Yeah but this seems an argument for having a clean solution for memory UMA,
where we can get all the data, including cc and gpu categories.

In summary: I am not too excited by drilling holes in the codebase to get
some half baked data which will probably cause problems to other projects.
If you want to do something with very short term, at least keep it self
contained in a file which we can drop entirely later.


>
>
>
> https://codereview.chromium.org/2350423003/
>
> --
> You received this message because you are subscribed to the Google Groups
> "tracing" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to tracing+unsubscribe@chromium.org.
> To post to this group, send email to tracing@chromium.org.
> To view this discussion on the web visit https://groups.google.com/a/
> chromium.org/d/msgid/tracing/047d7beb995c511c09053d627a5e%40google.com
>
<https://groups.google.com/a/chromium.org/d/msgid/tracing/047d7beb995c511c0905...>
> .
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Primiano Tucci (use gerrit), 2016-09-26 18:34:58]

On Mon, Sep 26, 2016 at 6:34 AM, tasak via tracing <tracing@chromium.org>
wrote:

> I would like to confirm. Your suggestion is that it is better to re-use the
> exising UMAs: Memory.Renderer.Large2, ... and so on for purge+suspend?
>
Well, the code that they call. You definitely need to create some new
histograms to check before/after.
What I am saying here is that instead of rushing up something that will
give: (i) harder maintenance (ii) more codereview traffic (iii) an
intermediate solution (you'll get only few of the memory-infra columns
anyways), let's start with a v0 which gives you something simple and clean
(the current UMA histograms), and let's shoot for doing the right thing in
v1.


> (1) Because the UMAs (and ProcessData used to generate the UMAs) are only
> available for browser process.
>
> Since the UMAs are reported in chrome/browser/metrics/
> metrics_memory_details.cc,
> only browser process can report the UMAs, I think.
> However purge+suspend is implemented in renderer processes.
> So if we want to re-use the UMAs, we probably need to send some IPC
> message from
> renderer process to browser process.
> I'm not sure whether the messaging code is simple or not.
>

If I read the code correctly, all that stuff just uses
base::ProcessMetrics::GetWorkingSetKBytesStatm at the end.
Can you use the same in your UMA from the renderer?


> (2) Because in the near future I would like to replace the hacky code with
> memory-infra.
>
> I expect that it will be possible to invoke memory-infra without tracing
> in the
> near future.
>
Yes, that's the plan.


> Since my hacky code is based on OnMemoryDump, I expect that we can
> compare the hacky purge+suspend UMAs with UMAs using memory-infra.
>
Yeah but that hacky OnMemoryDump will very likely interfere with things
like the existing slow-reports and general use of tracing. If it starts
randomly crashing perf benchmarks on telemetry, or people trying to use
chrome://tracing it will be a pain to debug those races.



> (3) Because I would like to see each allocator's memory usage if possible.
>
Can you add just a call to mallinfo()? That will be a temporary but
"self-contained" solution, which will not affect other pieces of the
codebase.


> If we have the data, at least it will be possible to see whether "purge" is
> working well or not.
> For example, I have already found that sometimes "malloc" memory usage
> increases
> because it is not possible to suspend compositor-related workers.
>
Yeah but this seems an argument for having a clean solution for memory UMA,
where we can get all the data, including cc and gpu categories.

In summary: I am not too excited by drilling holes in the codebase to get
some half baked data which will probably cause problems to other projects.
If you want to do something with very short term, at least keep it self
contained in a file which we can drop entirely later.


>
>
>
> https://codereview.chromium.org/2350423003/
>
> --
> You received this message because you are subscribed to the Google Groups
> "tracing" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to tracing+unsubscribe@chromium.org.
> To post to this group, send email to tracing@chromium.org.
> To view this discussion on the web visit https://groups.google.com/a/
> chromium.org/d/msgid/tracing/047d7beb995c511c09053d627a5e%40google.com
>
<https://groups.google.com/a/chromium.org/d/msgid/tracing/047d7beb995c511c0905...>
> .
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tasak, 2016-09-28 09:29:50]

The CQ bit was checked by tasak@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 09:30:01]

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-09-28 09:42:51]

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

[commit-bot: I haz the power, 2016-09-28 09:42:52]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[tasak, 2016-09-28 09:55:04]

I found that a finch experiment supports A/B testing.
So I modified UMAs, i.e. replaced difference of memory usages with current
memory usages.

On 2016/09/26 18:34:58, Primiano Tucci wrote:
> On Mon, Sep 26, 2016 at 6:34 AM, tasak via tracing
<mailto:tracing@chromium.org>
> wrote:

> Well, the code that they call. You definitely need to create some new
> histograms to check before/after.
> What I am saying here is that instead of rushing up something that will
> give: (i) harder maintenance (ii) more codereview traffic (iii) an
> intermediate solution (you'll get only few of the memory-infra columns
> anyways), let's start with a v0 which gives you something simple and clean
> (the current UMA histograms), and let's shoot for doing the right thing in
> v1.


So I think, it depends on when v0 is landed.
If v0 is landed soon, yeah, I would like to follow the suggested way.


> If I read the code correctly, all that stuff just uses
> base::ProcessMetrics::GetWorkingSetKBytesStatm at the end.
> Can you use the same in your UMA from the renderer?

I think, ProcessMetrics information is not helpful to understand how
Purge+Suspend 
works by looking at real user data.
The short term goal of our finch experiment is to know much about Purge+Suspend,
i.e. whether it really reduces PartitionAlloc, BlinkGC, ... memory usages or
not, and
whether it breaks web or not.

When we decide whether to ship Purge+Suspend, the ProcessMetrics information is
useful...
However firstly we need at least per-allocation memory usage.

> Yeah but that hacky OnMemoryDump will very likely interfere with things
> like the existing slow-reports and general use of tracing. If it starts
> randomly crashing perf benchmarks on telemetry, or people trying to use
> chrome://tracing it will be a pain to debug those races.

So your concern is malloc_statistics.{h,cc}?
I reverted the code and copied some code to render_thread_impl.cc.
So the patch doesn't affect the things, I think.

> Can you add just a call to mallinfo()? That will be a temporary but
> "self-contained" solution, which will not affect other pieces of the
> codebase.

Done. However, mallinfo() doesn't work on Windows and MacOS.
I added HeapWalker and malloc_zone code to render_thread_impl.cc.

> Yeah but this seems an argument for having a clean solution for memory UMA,
> where we can get all the data, including cc and gpu categories.

No, I have no argument.
I would like to say, we don't know when MemoryCoordinator v0 is landed.
So unfortunately we need to see malloc/discardable memory usage to understand
cc/gpu.

> In summary: I am not too excited by drilling holes in the codebase to get
> some half baked data which will probably cause problems to other projects.
> If you want to do something with very short term, at least keep it self
> contained in a file which we can drop entirely later.

Hmmm...
I think, BlinkGC, v8, PartitionAlloc have already had methods reporting memory
usages.
The patch just made the methods public.
So I would like to understand how the patch causes problems to other projects to
improve the patch.

[tasak, 2016-09-28 10:00:35]

Patchset #6 (id:140001) has been deleted

[tasak, 2016-09-28 10:00:43]

The CQ bit was checked by tasak@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 10:00:57]

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-09-28 10:14:35]

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

[commit-bot: I haz the power, 2016-09-28 10:14:37]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Primiano Tucci (use gerrit), 2016-09-28 15:29:28]

> So I would like to understand how the patch causes problems to other projects
to
improve the patch.
Thanks PS6 looks less invasive in this regard. Has a a lot of duplicated code
that you had to copy over,  but considering that, as you say, it's a temporary
thing at least is not breaking the existing features.
 
If the owners of the folders you are touching here are fine with this I'm fine,
as long as we work on a decent solution on this and clean up all this mess.

[Sami, 2016-09-28 16:01:06]

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

[Sami, 2016-09-28 16:01:08]

https://codereview.chromium.org/2350423003/diff/160001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2350423003/diff/160001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1764: base::TimeDelta::FromSeconds(15));
What if the renderer is unsuspended before this task runs?

Does purging involve multiple posted tasks? If not, we could post a regular
non-delayed task which runs after the current one.

nit: comment says 5 seconds.

[Ilya Sherman, 2016-09-28 23:25:10]

Is this ready for me to review, or should I wait until some more basic questions
are resolved?

[tasak, 2016-09-29 02:37:51]

Thank you for review.

https://codereview.chromium.org/2350423003/diff/160001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2350423003/diff/160001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1764: base::TimeDelta::FromSeconds(15));
On 2016/09/28 16:01:08, Sami wrote:
> What if the renderer is unsuspended before this task runs?

OnDumpMemoryUsage checks is_renderer_suspended. So OnDumpMemoryUsage does
nothing if is_renderer_suspended is false.

> Does purging involve multiple posted tasks? If not, we could post a regular
> non-delayed task which runs after the current one.

Yeah, currently purging (depending on MemoryPressureListener) involves multiple
posted tasks and does not support reply callback.

> nit: comment says 5 seconds.

Thank you.
Done.

[haraken, 2016-09-29 02:39:04]

On 2016/09/29 02:37:51, tasak wrote:
> Thank you for review.
> 
>
https://codereview.chromium.org/2350423003/diff/160001/content/renderer/rende...
> File content/renderer/render_thread_impl.cc (right):
> 
>
https://codereview.chromium.org/2350423003/diff/160001/content/renderer/rende...
> content/renderer/render_thread_impl.cc:1764:
base::TimeDelta::FromSeconds(15));
> On 2016/09/28 16:01:08, Sami wrote:
> > What if the renderer is unsuspended before this task runs?
> 
> OnDumpMemoryUsage checks is_renderer_suspended. So OnDumpMemoryUsage does
> nothing if is_renderer_suspended is false.
> 
> > Does purging involve multiple posted tasks? If not, we could post a regular
> > non-delayed task which runs after the current one.
> 
> Yeah, currently purging (depending on MemoryPressureListener) involves
multiple
> posted tasks and does not support reply callback.
> 
> > nit: comment says 5 seconds.
> 
> Thank you.
> Done.

If primiano and sami are fine with PS7, I'm fine with PS7.

[tasak, 2016-09-29 02:47:42]

The CQ bit was checked by tasak@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 02:47:59]

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-09-29 03:16:53]

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

[commit-bot: I haz the power, 2016-09-29 03:16:54]

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

[tasak, 2016-09-29 04:31:04]

The CQ bit was checked by tasak@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 04:31:24]

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-09-29 04:52:35]

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

[commit-bot: I haz the power, 2016-09-29 04:52:36]

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

[tasak, 2016-09-29 08:36:33]

The CQ bit was checked by tasak@google.com to run a CQ dry run

[tasak, 2016-09-29 08:36:50]

Patchset #9 (id:220001) has been deleted

[commit-bot: I haz the power, 2016-09-29 08:36:52]

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-09-29 10:07:14]

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

[commit-bot: I haz the power, 2016-09-29 10:07:16]

Dry run: This issue passed the CQ dry run.

[tasak, 2016-09-30 08:11:22]

The CQ bit was checked by tasak@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 08:11:34]

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-09-30 10:12:02]

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

[commit-bot: I haz the power, 2016-09-30 10:12:03]

Dry run: Try jobs failed on following builders:
  android_s5_perf_cq on master.tryserver.chromium.perf (JOB_TIMED_OUT, no build
URL)
  mac_retina_perf_cq on master.tryserver.chromium.perf (JOB_TIMED_OUT, no build
URL)