Minor fixes to Mojo MemoryDumpProvider.

Minor fixes to Mojo MemoryDumpProvider.

  * Allow values from Mojo MemoryDumpProviders to be captured by slow reports by whitelisting them for background traces.
  * Add a unit test.
  * Fix destructor to correctly unregister the MemoryDumpProvider.

BUG=705785
Review-Url: https://codereview.chromium.org/2930533003
Cr-Commit-Position: refs/heads/master@{#478327}
Committed: https://chromium.googlesource.com/chromium/src/+/337e9da889a1982924a7d730ca3e8d6b0bc073d7

[erikchen, 2017-06-07 00:05:38]

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

[erikchen, 2017-06-07 00:05:39]

dskiba: Please review.

[DmitrySkiba, 2017-06-07 00:10:01]

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

[DmitrySkiba, 2017-06-07 00:10:02]

LGTM

+ssid to double-check.

[ssid, 2017-06-07 00:31:24]

On 2017/06/07 00:10:02, DmitrySkiba wrote:
> LGTM
> 
> +ssid to double-check.

Added some comments to the other CL. Please fix them before this CL.

[erikchen, 2017-06-07 02:56:56]

Description was changed from

==========
Allow value from Mojo MemoryDumpProviders to be captured by slow reports.

Whitelist the relevant fields for capturing during tracing in background mode.

BUG=705785
==========

to

==========
Minor fixes to Mojo MemoryDumpProvider.

  * Allow values from Mojo MemoryDumpProviders to be captured by slow reports by
whitelisting them for background traces.
  * Add a unit test.
  * Fix destructor to correctly unregister the MemoryDumpProvider.

BUG=705785
==========

[erikchen, 2017-06-07 02:57:39]

Description was changed from

==========
Minor fixes to Mojo MemoryDumpProvider.

  * Allow values from Mojo MemoryDumpProviders to be captured by slow reports by
whitelisting them for background traces.
  * Add a unit test.
  * Fix destructor to correctly unregister the MemoryDumpProvider.

BUG=705785
==========

to

==========
Minor fixes to Mojo MemoryDumpProvider.

  * Allow values from Mojo MemoryDumpProviders to be captured by slow reports by
whitelisting them for background traces.
  * Add a unit test.
  * Fix destructor to correctly unregister the MemoryDumpProvider.

BUG=705785
==========

[erikchen, 2017-06-07 02:58:28]

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

[erikchen, 2017-06-07 02:58:32]

ssid: PTAL

[commit-bot: I haz the power, 2017-06-07 02:58:42]

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-06-07 04:01:09]

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

[commit-bot: I haz the power, 2017-06-07 04:01:10]

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

[erikchen, 2017-06-08 04:36:48]

On 2017/06/07 02:58:32, erikchen wrote:
> ssid: PTAL

ssdi: PTAL

[Primiano Tucci (use gerrit), 2017-06-08 12:55:19]

RS-LGTM for 	base/trace_event/memory_infra_background_whitelist.cc once ssid is
happy.
I wish I could help more but you folks are talking about "other CL" and I have
no idea what it is :)

[ssid, 2017-06-08 18:12:37]

Thanks for fixing the provider.
The provider does not seem to add size at all. Is it just the first version of
the provider and we want to get some data?

The slow reports process currently only takes "size" parameter in the graphs. If
we do not have size the provider will not show up in the aggregates. I'd also
have to change the processing js script to emit these metrics, which means
changing the processing jobs (just dremel queries on the database won't work).

https://codereview.chromium.org/2930533003/diff/20001/base/trace_event/memory...
File base/trace_event/memory_infra_background_whitelist.cc (right):

https://codereview.chromium.org/2930533003/diff/20001/base/trace_event/memory...
base/trace_event/memory_infra_background_whitelist.cc:40: "MojoHandleTable",
nit: please place it in order, move to line 31

https://codereview.chromium.org/2930533003/diff/20001/mojo/edk/system/core.cc
File mojo/edk/system/core.cc (right):

https://codereview.chromium.org/2930533003/diff/20001/mojo/edk/system/core.cc...
mojo/edk/system/core.cc:148:
->UnregisterAndDeleteDumpProviderSoon(std::move(handles_));
Can we clear the memory used by |handles_| befoe passing to MDM?

{
  base::AutoLock lock(handles_->GetLock());
  handles_->handles_.clear();
}

Just to make sure that we don't keep around extra memory just because
memory-infra is hung / taking time.

https://codereview.chromium.org/2930533003/diff/20001/mojo/edk/system/handle_...
File mojo/edk/system/handle_table.cc (right):

https://codereview.chromium.org/2930533003/diff/20001/mojo/edk/system/handle_...
mojo/edk/system/handle_table.cc:169:
handle_count[Dispatcher::Type::PLATFORM_HANDLE];
Are these lines required? The last time I tested just the statement below works
fine.

[erikchen, 2017-06-09 00:18:12]

https://codereview.chromium.org/2930533003/diff/20001/base/trace_event/memory...
File base/trace_event/memory_infra_background_whitelist.cc (right):

https://codereview.chromium.org/2930533003/diff/20001/base/trace_event/memory...
base/trace_event/memory_infra_background_whitelist.cc:40: "MojoHandleTable",
On 2017/06/08 18:12:36, ssid wrote:
> nit: please place it in order, move to line 31

Done.

https://codereview.chromium.org/2930533003/diff/20001/mojo/edk/system/core.cc
File mojo/edk/system/core.cc (right):

https://codereview.chromium.org/2930533003/diff/20001/mojo/edk/system/core.cc...
mojo/edk/system/core.cc:148:
->UnregisterAndDeleteDumpProviderSoon(std::move(handles_));
On 2017/06/08 18:12:36, ssid wrote:
> Can we clear the memory used by |handles_| befoe passing to MDM?
> 
> {
>   base::AutoLock lock(handles_->GetLock());
>   handles_->handles_.clear();
> }
> 
> Just to make sure that we don't keep around extra memory just because
> memory-infra is hung / taking time.
>   

Sorry, I feel strongly about *not* doing what you suggested. 

Either this is a serious issue that should be indicated in the comments and
uniformly followed by all MDPs that invoke this method, presumably with
investigation to figure out why it's hung/taking time to fix it, or else we
shouldn't worry about this and assume that the MemoryDumpManager is functioning
appropriately.

"defensive programming" against potential issues clutters code without
[frequently] addressing the root cause of the potential issues.

https://codereview.chromium.org/2930533003/diff/20001/mojo/edk/system/handle_...
File mojo/edk/system/handle_table.cc (right):

https://codereview.chromium.org/2930533003/diff/20001/mojo/edk/system/handle_...
mojo/edk/system/handle_table.cc:169:
handle_count[Dispatcher::Type::PLATFORM_HANDLE];
On 2017/06/08 18:12:36, ssid wrote:
> Are these lines required? The last time I tested just the statement below
works
> fine.

As I commented above, this causes the entries to be created in the map, which
causes us to emit "0" for entries that otherwise would be omitted entirely.

[erikchen, 2017-06-09 00:18:18]

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

[commit-bot: I haz the power, 2017-06-09 00:18:51]

Dry run: CQ is trying da patch.

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

[ssid, 2017-06-09 00:38:28]

thanks

https://codereview.chromium.org/2930533003/diff/20001/mojo/edk/system/core.cc
File mojo/edk/system/core.cc (right):

https://codereview.chromium.org/2930533003/diff/20001/mojo/edk/system/core.cc...
mojo/edk/system/core.cc:148:
->UnregisterAndDeleteDumpProviderSoon(std::move(handles_));
On 2017/06/09 00:18:12, erikchen wrote:
> On 2017/06/08 18:12:36, ssid wrote:
> > Can we clear the memory used by |handles_| befoe passing to MDM?
> > 
> > {
> >   base::AutoLock lock(handles_->GetLock());
> >   handles_->handles_.clear();
> > }
> > 
> > Just to make sure that we don't keep around extra memory just because
> > memory-infra is hung / taking time.
> >   
> 
> Sorry, I feel strongly about *not* doing what you suggested. 
> 
> Either this is a serious issue that should be indicated in the comments and

The comment on the api says "The |mdp| will be deleted at some point in the near
future." which means it could take a long time too.

> uniformly followed by all MDPs that invoke this method, presumably with
> investigation to figure out why it's hung/taking time to fix it, or else we
> shouldn't worry about this and assume that the MemoryDumpManager is
functioning
> appropriately.
> 
> "defensive programming" against potential issues clutters code without
> [frequently] addressing the root cause of the potential issues.

It need not be just the case where memory-infra hangs. It is a possible scenario
where the main threads are busy and the background thread of memory-infra never
got to execute tasks for a long time. This would still mean we keep around extra
memory for that time, especially when the table can be big.

I do not think this is defensive programming, i think it is more efficient
programming.

https://codereview.chromium.org/2930533003/diff/20001/mojo/edk/system/handle_...
File mojo/edk/system/handle_table.cc (right):

https://codereview.chromium.org/2930533003/diff/20001/mojo/edk/system/handle_...
mojo/edk/system/handle_table.cc:169:
handle_count[Dispatcher::Type::PLATFORM_HANDLE];
On 2017/06/09 00:18:12, erikchen wrote:
> On 2017/06/08 18:12:36, ssid wrote:
> > Are these lines required? The last time I tested just the statement below
> works
> > fine.
> 
> As I commented above, this causes the entries to be created in the map, which
> causes us to emit "0" for entries that otherwise would be omitted entirely.

Acknowledged.

https://codereview.chromium.org/2930533003/diff/40001/mojo/edk/system/handle_...
File mojo/edk/system/handle_table.cc (right):

https://codereview.chromium.org/2930533003/diff/40001/mojo/edk/system/handle_...
mojo/edk/system/handle_table.cc:179: pmd->CreateAllocatorDump("mojo");
This line is not required.

[ssid, 2017-06-09 00:40:25]

On 2017/06/08 12:55:19, Primiano Tucci wrote:
> RS-LGTM for 	base/trace_event/memory_infra_background_whitelist.cc once ssid
is
> happy.
> I wish I could help more but you folks are talking about "other CL" and I have
> no idea what it is :)

Sorry link: https://codereview.chromium.org/2915013006/

[ssid, 2017-06-09 00:42:02]

@Eric:
The provider does not seem to add size at all. Is it just the first version of
the provider and we want to start getting any data? How are you planning to use
this?

The slow reports process currently only takes "size" parameter in the graphs. If
we do not have size the provider will not show up in the aggregates. I'd also
have to change the processing js script to emit these metrics, which means
changing the processing jobs (just dremel queries on the database won't work).

[commit-bot: I haz the power, 2017-06-09 01:37:28]

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

[commit-bot: I haz the power, 2017-06-09 01:37:29]

Dry run: This issue passed the CQ dry run.

[erikchen, 2017-06-09 02:06:15]

On 2017/06/09 00:42:02, ssid wrote:
> @Eric:
> The provider does not seem to add size at all. Is it just the first version of
> the provider and we want to start getting any data? How are you planning to
use
> this?
I'd like to be able to count number of mojo handles in memory-infra dumps. 

> 
> The slow reports process currently only takes "size" parameter in the graphs.
If
> we do not have size the provider will not show up in the aggregates. I'd also
> have to change the processing js script to emit these metrics, which means
> changing the processing jobs (just dremel queries on the database won't work).
I only added slow reports code at dskiba's request. I'm happy to remove it as
well.

[erikchen, 2017-06-09 02:06:25]

https://codereview.chromium.org/2930533003/diff/20001/mojo/edk/system/core.cc
File mojo/edk/system/core.cc (right):

https://codereview.chromium.org/2930533003/diff/20001/mojo/edk/system/core.cc...
mojo/edk/system/core.cc:148:
->UnregisterAndDeleteDumpProviderSoon(std::move(handles_));

> The comment on the api says "The |mdp| will be deleted at some point in the
near
> future." which means it could take a long time too.
I read "near future" as not taking a long time.

> 
> > uniformly followed by all MDPs that invoke this method, presumably with
> > investigation to figure out why it's hung/taking time to fix it, or else we
> > shouldn't worry about this and assume that the MemoryDumpManager is
> functioning
> > appropriately.
> > 
> > "defensive programming" against potential issues clutters code without
> > [frequently] addressing the root cause of the potential issues.
> 
> It need not be just the case where memory-infra hangs. It is a possible
scenario
> where the main threads are busy and the background thread of memory-infra
never
> got to execute tasks for a long time. This would still mean we keep around
extra
> memory for that time, especially when the table can be big.
> 
> I do not think this is defensive programming, i think it is more efficient
> programming.
I guess we disagree about this point. I'm happy to do whatever rockot@ prefers.

[ssid, 2017-06-09 07:17:20]

On 2017/06/09 02:06:25, erikchen wrote:
> https://codereview.chromium.org/2930533003/diff/20001/mojo/edk/system/core.cc
> File mojo/edk/system/core.cc (right):
> 
>
https://codereview.chromium.org/2930533003/diff/20001/mojo/edk/system/core.cc...
> mojo/edk/system/core.cc:148:
> ->UnregisterAndDeleteDumpProviderSoon(std::move(handles_));
> 
> > The comment on the api says "The |mdp| will be deleted at some point in the
> near
> > future." which means it could take a long time too.
> I read "near future" as not taking a long time.
> 
> > 
> > > uniformly followed by all MDPs that invoke this method, presumably with
> > > investigation to figure out why it's hung/taking time to fix it, or else
we
> > > shouldn't worry about this and assume that the MemoryDumpManager is
> > functioning
> > > appropriately.
> > > 
> > > "defensive programming" against potential issues clutters code without
> > > [frequently] addressing the root cause of the potential issues.
> > 
> > It need not be just the case where memory-infra hangs. It is a possible
> scenario
> > where the main threads are busy and the background thread of memory-infra
> never
> > got to execute tasks for a long time. This would still mean we keep around
> extra
> > memory for that time, especially when the table can be big.
> > 
> > I do not think this is defensive programming, i think it is more efficient
> > programming.
> I guess we disagree about this point. I'm happy to do whatever rockot@
prefers.

Okay lgtm for other bits. Don't feel so strongly about clearing the map.

[Primiano Tucci (use gerrit), 2017-06-09 11:29:57]

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

[Primiano Tucci (use gerrit), 2017-06-09 11:29:58]

https://codereview.chromium.org/2930533003/diff/20001/mojo/edk/system/core.cc
File mojo/edk/system/core.cc (right):

https://codereview.chromium.org/2930533003/diff/20001/mojo/edk/system/core.cc...
mojo/edk/system/core.cc:148:
->UnregisterAndDeleteDumpProviderSoon(std::move(handles_));
On 2017/06/09 02:06:25, erikchen wrote:
> 
> > The comment on the api says "The |mdp| will be deleted at some point in the
> near
> > future." which means it could take a long time too.
> I read "near future" as not taking a long time.
> 
> > 
> > > uniformly followed by all MDPs that invoke this method, presumably with
> > > investigation to figure out why it's hung/taking time to fix it, or else
we
> > > shouldn't worry about this and assume that the MemoryDumpManager is
> > functioning
> > > appropriately.
> > > 
> > > "defensive programming" against potential issues clutters code without
> > > [frequently] addressing the root cause of the potential issues.
> > 
> > It need not be just the case where memory-infra hangs. It is a possible
> scenario
> > where the main threads are busy and the background thread of memory-infra
> never
> > got to execute tasks for a long time. This would still mean we keep around
> extra
> > memory for that time, especially when the table can be big.
> > 
> > I do not think this is defensive programming, i think it is more efficient
> > programming.
> I guess we disagree about this point. I'm happy to do whatever rockot@
prefers.

Hmm I am no OWNERz of this file but I lean towards Erik's way of thinking here.
That UnregisterAndDeleteDumpProviderSoon at most should defer the deletion to
the time it takes to complete the currently outstanding dump (if any).
Essentially is there to prevent that we delete the MDP while we iterate on it,
and guarantees linearization of the dtor.
So realistically I expect that object to be freed in <1s or so.
If we have issues of the form "the object is not really deleted soon" that is a
bug and we should fix the bug, not work around that to minimize the impact.
If there are cases where the dump can get stuck (probably there are) we should
introduce a timeout mechanism to recover and blacklist (I think we should do
that regardless, today we rely too much on MDP behaving sanely)

[ssid, 2017-06-09 14:22:28]

On 2017/06/09 11:29:58, Primiano Tucci wrote:
> https://codereview.chromium.org/2930533003/diff/20001/mojo/edk/system/core.cc
> File mojo/edk/system/core.cc (right):
> 
>
https://codereview.chromium.org/2930533003/diff/20001/mojo/edk/system/core.cc...
> mojo/edk/system/core.cc:148:
> ->UnregisterAndDeleteDumpProviderSoon(std::move(handles_));
> On 2017/06/09 02:06:25, erikchen wrote:
> > 
> > > The comment on the api says "The |mdp| will be deleted at some point in
the
> > near
> > > future." which means it could take a long time too.
> > I read "near future" as not taking a long time.
> > 
> > > 
> > > > uniformly followed by all MDPs that invoke this method, presumably with
> > > > investigation to figure out why it's hung/taking time to fix it, or else
> we
> > > > shouldn't worry about this and assume that the MemoryDumpManager is
> > > functioning
> > > > appropriately.
> > > > 
> > > > "defensive programming" against potential issues clutters code without
> > > > [frequently] addressing the root cause of the potential issues.
> > > 
> > > It need not be just the case where memory-infra hangs. It is a possible
> > scenario
> > > where the main threads are busy and the background thread of memory-infra
> > never
> > > got to execute tasks for a long time. This would still mean we keep around
> > extra
> > > memory for that time, especially when the table can be big.
> > > 
> > > I do not think this is defensive programming, i think it is more efficient
> > > programming.
> > I guess we disagree about this point. I'm happy to do whatever rockot@
> prefers.
> 
> Hmm I am no OWNERz of this file but I lean towards Erik's way of thinking
here.
> That UnregisterAndDeleteDumpProviderSoon at most should defer the deletion to
> the time it takes to complete the currently outstanding dump (if any).
> Essentially is there to prevent that we delete the MDP while we iterate on it,
> and guarantees linearization of the dtor.
> So realistically I expect that object to be freed in <1s or so.
> If we have issues of the form "the object is not really deleted soon" that is
a
> bug and we should fix the bug, not work around that to minimize the impact.
> If there are cases where the dump can get stuck (probably there are) we should
> introduce a timeout mechanism to recover and blacklist (I think we should do
> that regardless, today we rely too much on MDP behaving sanely)

Um, what I was suggesting was let's not keep the memory alive even for the 1
second. In no way I meant to say we should let go of bugs. I felt keeping like
10mb around for a second more than necessary could be avoided.

[erikchen, 2017-06-09 14:52:08]

erikchen@chromium.org changed reviewers:
+ rockot@chromium.org

[erikchen, 2017-06-09 14:52:09]

rockot: Please review.

[Ken Rockot(use gerrit already), 2017-06-09 15:11:51]

lgtm

[erikchen, 2017-06-09 16:25:32]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-06-09 16:25:33]

The patchset sent to the CQ was uploaded after l-g-t-m from dskiba@chromium.org,
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/2930533003/#ps60001
(title: "comments from ssid.")

[commit-bot: I haz the power, 2017-06-09 16:25:56]

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-06-09 17:49:47]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497025532560600,
"parent_rev": "e4815989660c3ef60e99b55d9b936246714e4719", "commit_rev":
"337e9da889a1982924a7d730ca3e8d6b0bc073d7"}

[commit-bot: I haz the power, 2017-06-09 17:49:58]

Description was changed from

==========
Minor fixes to Mojo MemoryDumpProvider.

  * Allow values from Mojo MemoryDumpProviders to be captured by slow reports by
whitelisting them for background traces.
  * Add a unit test.
  * Fix destructor to correctly unregister the MemoryDumpProvider.

BUG=705785
==========

to

==========
Minor fixes to Mojo MemoryDumpProvider.

  * Allow values from Mojo MemoryDumpProviders to be captured by slow reports by
whitelisting them for background traces.
  * Add a unit test.
  * Fix destructor to correctly unregister the MemoryDumpProvider.

BUG=705785

Review-Url: https://codereview.chromium.org/2930533003
Cr-Commit-Position: refs/heads/master@{#478327}
Committed:
https://chromium.googlesource.com/chromium/src/+/337e9da889a1982924a7d730ca3e...
==========

[commit-bot: I haz the power, 2017-06-09 17:50:01]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/337e9da889a1982924a7d730ca3e...