[Tentaive patch for discussion] Add Purge+Suspend metrics as UMA. (issue 2350423003 by tasak@google.com)

[ta...@google.com]

Reviewers: haraken, bashi1 (slow til Sep 26), Primiano Tucci
CL: https://codereview.chromium.org/2350423003/

Description:
[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

Affected files (+343, -29 lines):
M base/trace_event/malloc_dump_provider.h
M base/trace_event/malloc_dump_provider.cc
M content/child/child_discardable_shared_memory_manager.h
M content/child/child_discardable_shared_memory_manager.cc
M content/renderer/render_thread_impl.h
M content/renderer/render_thread_impl.cc
M third_party/WebKit/Source/web/BUILD.gn
A third_party/WebKit/Source/web/WebMemoryStatistics.cpp
M third_party/WebKit/public/BUILD.gn
A third_party/WebKit/public/web/WebMemoryStatistics.h
M tools/metrics/histograms/histograms.xml

[har...@chromium.org]

+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_thread_impl.cc
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2350423003/diff/1/content/renderer/render_thread_impl.cc#newcode1817
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/web/WebMemoryStatistics.cpp
File third_party/WebKit/Source/web/WebMemoryStatistics.cpp (right):

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

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

https://codereview.chromium.org/2350423003/

[ta...@google.com]

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

https://codereview.chromium.org/2350423003/diff/1/third_party/WebKit/Source/web/WebMemoryStatistics.cpp#newcode50
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.

https://codereview.chromium.org/2350423003/

[har...@chromium.org]

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.


https://codereview.chromium.org/2350423003/

[ishe...@chromium.org]

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

https://codereview.chromium.org/2350423003/diff/1/content/renderer/render_thread_impl.cc#newcode1817
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.

https://codereview.chromium.org/2350423003/

[ba...@chromium.org]

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

https://codereview.chromium.org/2350423003/diff/60001/base/trace_event/malloc_dump_provider.h#newcode52
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_discardable_shared_memory_manager.cc
File content/child/child_discardable_shared_memory_manager.cc (right):

https://codereview.chromium.org/2350423003/diff/60001/content/child/child_discardable_shared_memory_manager.cc#newcode238
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_thread_impl.cc
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2350423003/diff/60001/content/renderer/render_thread_impl.cc#newcode1763
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/Source/web/WebMemoryStatistics.cpp
File third_party/WebKit/Source/web/WebMemoryStatistics.cpp (right):

https://codereview.chromium.org/2350423003/diff/60001/third_party/WebKit/Source/web/WebMemoryStatistics.cpp#newcode40
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.

https://codereview.chromium.org/2350423003/

[prim...@chromium.org]

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_thread_impl.cc
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2350423003/diff/80001/content/renderer/render_thread_impl.cc#newcode1764
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?

https://codereview.chromium.org/2350423003/

[ta...@google.com]

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_thread_impl.cc
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2350423003/diff/80001/content/renderer/render_thread_impl.cc#newcode1764
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...

https://codereview.chromium.org/2350423003/

[prim...@chromium.org]

> 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_thread_impl.cc
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2350423003/diff/80001/content/renderer/render_thread_impl.cc#newcode1764
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.

https://codereview.chromium.org/2350423003/

[ta...@google.com]

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



https://codereview.chromium.org/2350423003/

[Primiano Tucci]

On Mon, Sep 26, 2016 at 6:34 AM, tasak via tracing <tra...@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 tra...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/tracing/047d7beb995c511c09053d627a5e%40google.com.

[ta...@google.com]

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:tra...@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.



https://codereview.chromium.org/2350423003/

[prim...@chromium.org]

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

https://codereview.chromium.org/2350423003/

[skyostil@chromium.org via codereview.chromium.org]

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

https://codereview.chromium.org/2350423003/diff/160001/content/renderer/render_thread_impl.cc#newcode1764
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.

https://codereview.chromium.org/2350423003/

[ishe...@chromium.org]

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

https://codereview.chromium.org/2350423003/

[ta...@google.com]

Thank you for review.

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

https://codereview.chromium.org/2350423003/diff/160001/content/renderer/render_thread_impl.cc#newcode1764
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.

https://codereview.chromium.org/2350423003/

[har...@chromium.org]

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


https://codereview.chromium.org/2350423003/