[memory-infra] Add support for polling memory totals in MemoryDumpManager

[memory-infra] Add support for polling memory totals in MemoryDumpManager

This adds new method in MemoryDumpProvider interface PollFastMemoryTotal
which is used for polling memory totals in process. The dump provider
has to enable this feature while registration.
For Simplicity of polling logic the dump providers supporting polling
must not have task runner affinity. Polling will be done on dump thread.

BUG=607533
Committed: https://crrev.com/4460b9b6393448adc011207ae63380a70ddba369
Cr-Commit-Position: refs/heads/master@{#439390}

[ssid, 2016-11-30 23:05:19]

Patchset #2 (id:20001) has been deleted

[ssid, 2016-11-30 23:05:23]

Patchset #1 (id:1) has been deleted

[ssid, 2016-12-01 07:13:48]

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

[commit-bot: I haz the power, 2016-12-01 07:14: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-12-01 07:37:18]

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

[commit-bot: I haz the power, 2016-12-01 07:37:19]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  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-...)

[ssid, 2016-12-01 17:50:23]

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

[commit-bot: I haz the power, 2016-12-01 17:50:43]

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-12-01 18:32:02]

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

[commit-bot: I haz the power, 2016-12-01 18:32:03]

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

[ssid, 2016-12-01 18:33:46]

Patchset #1 (id:40001) has been deleted

[ssid, 2016-12-02 00:33:40]

Patchset #1 (id:60001) has been deleted

[ssid, 2016-12-02 00:34:12]

Patchset #1 (id:80001) has been deleted

[ssid, 2016-12-02 01:49:08]

Patchset #1 (id:100001) has been deleted

[ssid, 2016-12-02 02:06:51]

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

[ssid, 2016-12-02 02:07:06]

Patchset #1 (id:120001) has been deleted

[commit-bot: I haz the power, 2016-12-02 02:07:19]

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

[ssid, 2016-12-02 02:09:36]

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

[ssid, 2016-12-02 02:09:37]

ptal, thanks. This cl only adds GetFastMemoryTotals api to MDM. This is not yet
used.

[commit-bot: I haz the power, 2016-12-02 02:58:40]

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

[commit-bot: I haz the power, 2016-12-02 02:58:41]

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

[Primiano Tucci (use gerrit), 2016-12-08 16:16:22]

> ptal, thanks. This cl only adds GetFastMemoryTotals api to MDM. This is not
yet
> used.

Ok this is a bit of a big change. Can we split it in parts so becomes more
manageable?
My suggestion is:

CL 1: Send a CL that changes only ProcessMetricsMemoryDumpProvider and adds:
  - SetFastMemoryPollingEnabled(bool)  -> this will open/close the statm file
  - size_t PollFastMemoryTotal() -> reads and returns the value

CL 2: Now the challenge here is how do we wire it up to MDM. As we discussed
offline this is a bit tricky because of the sandboxing model. In some cases
(Android and other OSs) each process can poll itself. In other (Linux desktop)
we poll all from the browser side.
I would use the principle: keep things as simple as possible unless you need
them complex.
In other words, I'd do the following:
- Okay to keep a parallel list of mdps_for_fast_polling_. Although only one
should suffice
- Assume that the fast polling MDPs are not thread-bound. That's the case for
the process_metrics stuff so no need to generalize it. (Add a check at
registration time of the form (CHECK(!supports_fast_polling ||
task_runner==null) << "we support fast polling only on unbound MDPs"
-  This assumption should make your code way less complicated. Because now the
thread where you poll and destroy is the same, so no need to keep extra copies
and do extra hops.

When we'll get to the graphics part we'll ask the graphics dump provider to
store something in an atomic variable that we'll get on the MDP thread. Or we'll
figure out something. 

Still the thing that makes me nervous (to be clear: is not you, it is all this
sandbox situation) is that we'll need to maintain different codepaths for
polling either all from browser side, or from each process. But let's do
incremental steps and let's push this problem for later.

Does the rest sound sensible?

[ssid, 2016-12-08 18:58:12]

On 2016/12/08 16:16:22, Primiano Tucci wrote:
> > ptal, thanks. This cl only adds GetFastMemoryTotals api to MDM. This is not
> yet
> > used.
> 
> Ok this is a bit of a big change. Can we split it in parts so becomes more
> manageable?
Sounds good.

> My suggestion is:
> 
> CL 1: Send a CL that changes only ProcessMetricsMemoryDumpProvider and adds:
>   - SetFastMemoryPollingEnabled(bool)  -> this will open/close the statm file
>   - size_t PollFastMemoryTotal() -> reads and returns the value

This sounds more like cl2 rather than cl1. The implementation of the api
provided by MDP (if we go for introducing api) should be done after the api
changes. 


> CL 2: Now the challenge here is how do we wire it up to MDM. As we discussed
> offline this is a bit tricky because of the sandboxing model. In some cases
> (Android and other OSs) each process can poll itself. In other (Linux desktop)
> we poll all from the browser side.
> I would use the principle: keep things as simple as possible unless you need
> them complex.
> In other words, I'd do the following:
> - Okay to keep a parallel list of mdps_for_fast_polling_. Although only one
> should suffice

The only reason for 2 parallel lists is that the ProcessMetricMemoryDumpProvider
can be added and removed (in child host thread) when polling happens.
For polling to access the list of providers without taking lock, it needs to
have a list that is owned by dump thread. This list needs to be created and
destroyed along with dump thread. The RegisterDumpProvider posts tasks to dump
thread to update the list when needed (so polling does not need lock).

> - Assume that the fast polling MDPs are not thread-bound. That's the case for
> the process_metrics stuff so no need to generalize it. (Add a check at
> registration time of the form (CHECK(!supports_fast_polling ||
> task_runner==null) << "we support fast polling only on unbound MDPs"

+1 to this. I just thought it is not a lot of work to support polling with a
given task runner. But I will remove this support making it only little simpler.

> -  This assumption should make your code way less complicated. Because now the
> thread where you poll and destroy is the same, so no need to keep extra copies
> and do extra hops.

As i said earlier, the extra copies are not made because of polling on other
threads, the extra copies and post tasks are needed because the processes can
start and end when polling and polling should not take lock.
Note that the polling task should not take locks is an assumption that is made
by the RequestIdleCallback api that you were planning. Also without locks the
polling is twice faster (0.2 -> 0.1 ms). I think we can make some complexity if
we can support polling without locks.

> When we'll get to the graphics part we'll ask the graphics dump provider to
> store something in an atomic variable that we'll get on the MDP thread. Or
we'll
> figure out something. 

Yes, I was thinking the same. Just in case if this was not possible we can still
support polling on different thread. But, I can do this later when needed. I
added this support here only because if doesn't add a lot of complexity.

> Still the thing that makes me nervous (to be clear: is not you, it is all this
> sandbox situation) is that we'll need to maintain different codepaths for
> polling either all from browser side, or from each process. But let's do
> incremental steps and let's push this problem for later.

This shouldn't be a big issue. Since we are already in a state where
ProcessMetricMemoryDumpProvider registers in process where it works. just fixing
that when sandbox is changed should fix polling as well automatically.

Should I fix a meeting to discuss in detail?

[Primiano Tucci (use gerrit), 2016-12-09 16:35:20]

On 2016/12/08 18:58:12, ssid wrote:
> On 2016/12/08 16:16:22, Primiano Tucci wrote:
> > > ptal, thanks. This cl only adds GetFastMemoryTotals api to MDM. This is
not
> > yet
> > > used.
> > 
> > Ok this is a bit of a big change. Can we split it in parts so becomes more
> > manageable?
> Sounds good.
> 
> > My suggestion is:
> > 
> > CL 1: Send a CL that changes only ProcessMetricsMemoryDumpProvider and adds:
> >   - SetFastMemoryPollingEnabled(bool)  -> this will open/close the statm
file
> >   - size_t PollFastMemoryTotal() -> reads and returns the value
> 
> This sounds more like cl2 rather than cl1. The implementation of the api
> provided by MDP (if we go for introducing api) should be done after the api
> changes. 

SG. Order of CL1 and 2 is irrelevant, as long as they are independent.

> 
> 
> > CL 2: Now the challenge here is how do we wire it up to MDM. As we discussed
> > offline this is a bit tricky because of the sandboxing model. In some cases
> > (Android and other OSs) each process can poll itself. In other (Linux
desktop)
> > we poll all from the browser side.
> > I would use the principle: keep things as simple as possible unless you need
> > them complex.
> > In other words, I'd do the following:
> > - Okay to keep a parallel list of mdps_for_fast_polling_. Although only one
> > should suffice
> 
> The only reason for 2 parallel lists is that the
ProcessMetricMemoryDumpProvider
> can be added and removed (in child host thread) when polling happens.
> For polling to access the list of providers without taking lock, it needs to
> have a list that is owned by dump thread. This list needs to be created and
> destroyed along with dump thread. The RegisterDumpProvider posts tasks to dump
> thread to update the list when needed (so polling does not need lock).

So, wait, let me get this right, I think I rushed my comment.
I totally get the need of *one* extra list because, as you say, we don't want to
take locks on the MDP thread. 100% agree on this.
However in this CL you are introducing *hree* extra lists:
polling_supported_mdps_
mdps_for_polling_
and polling_task_runner_

I think they come from the over-complication of trying to handle arbitrary task
runners.

What I'm saying is that you just need *one extra* (which I suggested to call
mdps_for_fast_polling_).

> > - Assume that the fast polling MDPs are not thread-bound. That's the case
for
> > the process_metrics stuff so no need to generalize it. (Add a check at
> > registration time of the form (CHECK(!supports_fast_polling ||
> > task_runner==null) << "we support fast polling only on unbound MDPs"
> 
> +1 to this. I just thought it is not a lot of work to support polling with a
> given task runner. But I will remove this support making it only little
simpler.
> 
> > -  This assumption should make your code way less complicated. Because now
the
> > thread where you poll and destroy is the same, so no need to keep extra
copies
> > and do extra hops.
> 
> As i said earlier, the extra copies are not made because of polling on other
> threads, the extra copies and post tasks are needed because the processes can
> start and end when polling and polling should not take lock.
> Note that the polling task should not take locks is an assumption that is made
> by the RequestIdleCallback api that you were planning. Also without locks the
> polling is twice faster (0.2 -> 0.1 ms). I think we can make some complexity
if
> we can support polling without locks.

+1 to all this, but I think one extra copy is more than sufficient.
I imagine the final code to look as follows:

MDM::RegisterDumpProviderInternal(mdp) {
  if (mdp.supports_fast_polling) {
    DCHECK(mdp.task_runner == nullptr);
    PostTask(RegisterFastMDPOnMDPThread);
  }
}

UnregisterDumpProviderInternal(mdp) {
  PostTask(UnregisterCopyOnMDPThread);
}

> 
> > When we'll get to the graphics part we'll ask the graphics dump provider to
> > store something in an atomic variable that we'll get on the MDP thread. Or
> we'll
> > figure out something. 
> 
> Yes, I was thinking the same. Just in case if this was not possible we can
still
> support polling on different thread. But, I can do this later when needed. I
> added this support here only because if doesn't add a lot of complexity.
> 
> > Still the thing that makes me nervous (to be clear: is not you, it is all
this
> > sandbox situation) is that we'll need to maintain different codepaths for
> > polling either all from browser side, or from each process. But let's do
> > incremental steps and let's push this problem for later.
> 
> This shouldn't be a big issue. Since we are already in a state where
> ProcessMetricMemoryDumpProvider registers in process where it works. just
fixing
> that when sandbox is changed should fix polling as well automatically.
> 
> Should I fix a meeting to discuss in detail?
I think that CL1-2 (order doesn't matter) are non-controversial (unless I'm
missing still something).
Definitely let's plan a chat to decide how to use all this logic once CL1 and 2
are in.
I agree is not a big issue is more a matter of factoring that in in our
coordination logic for the peak detector.
But that is going to be CL3+ :)

[Primiano Tucci (use gerrit), 2016-12-09 16:35:53]

Ah also BUG=607533 plz :)

[ssid, 2016-12-12 22:12:42]

Patchset #3 (id:180001) has been deleted

[ssid, 2016-12-12 22:13:16]

Description was changed from

==========
[memory-infra] Add support for polling memory totals in MemoryDumpManager

This adds new method in MemoryDumpProvider interface GetFastMemoryTotal
which is used for polling memory totals in process. The dump provider
has to enable this feature while registration.
For Simplicity of polling logic the dump providers supporting polling
must be able to run on a single thread task runner. Polling will be done
on this thread.

BUG=
==========

to

==========
[memory-infra] Add support for polling memory totals in MemoryDumpManager

This adds new method in MemoryDumpProvider interface PollFastMemoryTotal
which is used for polling memory totals in process. The dump provider
has to enable this feature while registration.
For Simplicity of polling logic the dump providers supporting polling
must not have task runner affinity. Polling will be done on dump thread.

BUG=607533
==========

[ssid, 2016-12-12 22:20:21]

Patchset #3 (id:200001) has been deleted

[ssid, 2016-12-12 22:26:44]

Thanks for explanation, made changes as suggested.

[ssid, 2016-12-12 22:48:22]

Patchset #3 (id:220001) has been deleted

[ssid, 2016-12-12 23:31:30]

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

[commit-bot: I haz the power, 2016-12-12 23:33:27]

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-12-13 00:11:23]

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

[commit-bot: I haz the power, 2016-12-13 00:11:24]

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

[ssid, 2016-12-13 05:44:48]

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

[commit-bot: I haz the power, 2016-12-13 05:45: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-12-13 05:52:45]

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

[commit-bot: I haz the power, 2016-12-13 05:52:46]

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

[ssid, 2016-12-15 05:44:06]

Patchset #5 (id:280001) has been deleted

[ssid, 2016-12-15 05:44:23]

Patchset #3 (id:240001) has been deleted

[Primiano Tucci (use gerrit), 2016-12-15 16:45:01]

The threading logic makes sense to me. 
Some minor comment here and there.
Didn't have time to review the test yet, will do that soon.

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:288: if
(options.is_fast_polling_supported && dump_thread_) {
Can you add a comment here saying:
// the list of polling MDPs is populated OnTraceLogEnabled(). This code deals
with the case of a MDP capable of fast polling that is registered after the
OnTraceLogEnabled().

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:351:
dump_thread_->task_runner()->PostTask(
Add a DCHECK(take_mdp_ownership_and_delete_async) << "MemoryDumpProviders
capable of fast polling must NOT be thread bound and hence destroyed using
UnregisterAndDeleteDumpProviderSoon()"

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:636: #if DCHECK_IS_ON()
plz dcheck section and just add a comment.
There is a short time window where this DCHECK will crash, even if everything is
consistent, specifically, this sequence:
- OnTraceLogDisabled() is called
- It takes the lock and std::move(dump_thread_).
- Now dump_thread_ is nullptr, but the thread is still alive and MDM hasn't
requested it to stop yet.
- PolLFastMemoryTotal is called here, tries to access dump_thread_ and
segfaults.
- MDM continues the section outside of the lock in OnTraceLogDisabled, and
requests the thread (that in the meantime has been moved to a local variable) to
Stop()

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:770:
mdpinfo->dump_provider->SetFastMemoryPollingEnabled(true);
I think we should do this call only when needed.
Otherwise doesn't make lot of sense to have this method in the first place

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.h:333: // Records a quick total memory
usage value that can be polled in short
"that can be polled in short intervals" is quite redundant. the "quick" part
above should be enough.

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.h:337: void PollFastMemoryTotal(uint64_t*
memory_total);
I'd probably add also: this value is approximate to trade-off speed, and not
consistent with the rest of the memory-infra metrics.

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.h:350: // Adds / removes polling supported
provider to the copy
s/polling supported provider/providers that support fast polling/
also remove the "copy". I know what you meant there but IMHO is more confusing
than explanatory.

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.h:351: // |mdps_for_fast_polling_|. This
function should run on the dump thread so
No need for the "This function should run..." part. "OnDumpThread" as part of
the name should be pretty clear ;-)

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.h:369: MemoryDumpProviderInfo::OrderedSet
mdps_for_fast_polling_;
for readability and consistency I would:
move this just after dump_providers_
call it dumps_providers_for_polling_

[ssid, 2016-12-16 02:31:54]

made changes, thanks!

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:288: if
(options.is_fast_polling_supported && dump_thread_) {
On 2016/12/15 16:45:01, Primiano Tucci wrote:
> Can you add a comment here saying:
> // the list of polling MDPs is populated OnTraceLogEnabled(). This code deals
> with the case of a MDP capable of fast polling that is registered after the
> OnTraceLogEnabled().

Done.

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:351:
dump_thread_->task_runner()->PostTask(
On 2016/12/15 16:45:01, Primiano Tucci wrote:
> Add a DCHECK(take_mdp_ownership_and_delete_async) << "MemoryDumpProviders
> capable of fast polling must NOT be thread bound and hence destroyed using
> UnregisterAndDeleteDumpProviderSoon()"

Done.

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:636: #if DCHECK_IS_ON()
On 2016/12/15 16:45:01, Primiano Tucci wrote:
> plz dcheck section and just add a comment.
> There is a short time window where this DCHECK will crash, even if everything
is
> consistent, specifically, this sequence:
> - OnTraceLogDisabled() is called
> - It takes the lock and std::move(dump_thread_).
> - Now dump_thread_ is nullptr, but the thread is still alive and MDM hasn't
> requested it to stop yet.
> - PolLFastMemoryTotal is called here, tries to access dump_thread_ and
> segfaults.
> - MDM continues the section outside of the lock in OnTraceLogDisabled, and
> requests the thread (that in the meantime has been moved to a local variable)
to
> Stop()
> 
>  

I agree this is the case. But this is the part I should be worrying about in the
cl that actually polls. According to this CL, the polling could (theoretically)
only happen if dump_thread_ is present.
In my next cl i was planning to add a check here:
(if (dump_thread_) DCHECK())
since that condition does not make sense for this cl, i left it.
This section is useful for testing. but removed it now. In my next cl, i will
add it/ discuss if we need.

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:770:
mdpinfo->dump_provider->SetFastMemoryPollingEnabled(true);
On 2016/12/15 16:45:01, Primiano Tucci wrote:
> I think we should do this call only when needed.
> Otherwise doesn't make lot of sense to have this method in the first place

I just don't like the idea of session_state_ scanning the triggers once and the
PeriodicDumpTimer scanning it again. Wish I could put it all in one place.
Maybe move PeriodicDumpTimer and mdps_for_fast_polling_ into session_state_

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.h (right):

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.h:333: // Records a quick total memory
usage value that can be polled in short
On 2016/12/15 16:45:01, Primiano Tucci wrote:
> "that can be polled in short intervals" is quite redundant. the "quick" part
> above should be enough.

Done.

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.h:337: void PollFastMemoryTotal(uint64_t*
memory_total);
On 2016/12/15 16:45:01, Primiano Tucci wrote:
> I'd probably add also: this value is approximate to trade-off speed, and not
> consistent with the rest of the memory-infra metrics.

Done.

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.h:350: // Adds / removes polling supported
provider to the copy
On 2016/12/15 16:45:01, Primiano Tucci wrote:
> s/polling supported provider/providers that support fast polling/
> also remove the "copy". I know what you meant there but IMHO is more confusing
> than explanatory.

Done.

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.h:351: // |mdps_for_fast_polling_|. This
function should run on the dump thread so
On 2016/12/15 16:45:01, Primiano Tucci wrote:
> No need for the "This function should run..." part. "OnDumpThread" as part of
> the name should be pretty clear ;-)

Done.

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.h:369: MemoryDumpProviderInfo::OrderedSet
mdps_for_fast_polling_;
On 2016/12/15 16:45:01, Primiano Tucci wrote:
> for readability and consistency I would:
> move this just after dump_providers_
> call it dumps_providers_for_polling_ 
> 

Done.

[ssid, 2016-12-16 07:03:08]

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

[commit-bot: I haz the power, 2016-12-16 07:03:23]

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-12-16 10:16:07]

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

[commit-bot: I haz the power, 2016-12-16 10:16:08]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2016-12-16 12:34:40]

Ok final comments.
Had a second though on the all session state story, sorry for the back and
forth. Let's just keep it versy simple, not add anything to the session state,
and make the polling lazy by default (no need for SetEnabled, only notify the
disable. See the comments below)

Thanks for the patience, I'll LG all the CLs today once we fix this, I promise
:)

Also thanks for the test.

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:636: #if DCHECK_IS_ON()
On 2016/12/16 02:31:54, ssid wrote:
> On 2016/12/15 16:45:01, Primiano Tucci wrote:
> > plz dcheck section and just add a comment.
> > There is a short time window where this DCHECK will crash, even if
everything
> is
> > consistent, specifically, this sequence:
> > - OnTraceLogDisabled() is called
> > - It takes the lock and std::move(dump_thread_).
> > - Now dump_thread_ is nullptr, but the thread is still alive and MDM hasn't
> > requested it to stop yet.
> > - PolLFastMemoryTotal is called here, tries to access dump_thread_ and
> > segfaults.
> > - MDM continues the section outside of the lock in OnTraceLogDisabled, and
> > requests the thread (that in the meantime has been moved to a local
variable)
> to
> > Stop()
> > 
> >  
> 
> I agree this is the case. But this is the part I should be worrying about in
the
> cl that actually polls. According to this CL, the polling could
(theoretically)
> only happen if dump_thread_ is present.
> In my next cl i was planning to add a check here:
> (if (dump_thread_) DCHECK())
> since that condition does not make sense for this cl, i left it.
> This section is useful for testing. but removed it now. In my next cl, i will
> add it/ discuss if we need.

Let's move this discussion to the next CL.
There is no real race here. The thread is consistently alive when you are here,
The only problem is that you shouldn't never look at the state of MDM fields_
from the dumper threads, because there are moments when the state of MDM doesn't
match the reality.

https://codereview.chromium.org/2537363003/diff/300001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:770:
mdpinfo->dump_provider->SetFastMemoryPollingEnabled(true);
Ok the more I think to this the more I feel that the answer is:
get rid of SetFastMemoryPollingEnabled(true) and have the MDP lazily enable
(like you already do)
Let's turn this into a:

// Indicates that fast memory polling is not going to be used in the near future
and the MDP can tear down any resource kept around for fast memory polling.
void SuspendFastMemoryPolling()

Otherwise we are going to add complications everywhere for nothing.
Sorry for all the back and forth for this.

https://codereview.chromium.org/2537363003/diff/320001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2537363003/diff/320001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:358: "bound and hence destroyed using "
+"must be" before destroyed. Otherwise is not clear where the "destroyed" part
refers to "must NOT".

https://codereview.chromium.org/2537363003/diff/320001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:799: if
(!subtle::NoBarrier_Load(&memory_tracing_enabled_))
why is this required?

https://codereview.chromium.org/2537363003/diff/320001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:824: session_state_ = nullptr;
let's put this back in place (see my comment on lazy polling and avoiding
setEnabled)

https://codereview.chromium.org/2537363003/diff/320001/base/trace_event/memor...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/2537363003/diff/320001/base/trace_event/memor...
base/trace_event/memory_dump_manager_unittest.cc:157: ON_CALL(*this,
PollFastMemoryTotal(_))
I think you don't need this really, by default the behavior of gmock should be
to fail if a mocked method gets called unexpectedly

https://codereview.chromium.org/2537363003/diff/320001/base/trace_event/memor...
base/trace_event/memory_dump_manager_unittest.cc:784:
.WillRepeatedly(Invoke([](uint64_t* total) -> void {}));
I think you don't need this empty lambda.
Either you don't need the WillRepeatedly  part at all (by default it should do
nothing).
Or worst case if it doesn't build just do .WillRepeatedly(Return)

[ssid, 2016-12-16 18:58:22]

Fixed, thanks!

https://codereview.chromium.org/2537363003/diff/320001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2537363003/diff/320001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:358: "bound and hence destroyed using "
On 2016/12/16 12:34:40, Primiano Tucci wrote:
> +"must be" before destroyed. Otherwise is not clear where the "destroyed" part
> refers to "must NOT".

Done.

https://codereview.chromium.org/2537363003/diff/320001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:799: if
(!subtle::NoBarrier_Load(&memory_tracing_enabled_))
On 2016/12/16 12:34:40, Primiano Tucci wrote:
> why is this required?

Not really required, just an optimization to return faster.
In case tracing is enabled without "memory-infra", we will still be holding 2
locks and checking if out members are set. This can just be avoided if we never
enabled memory-infra.
Added this now because extra lock is added.

https://codereview.chromium.org/2537363003/diff/320001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:824: session_state_ = nullptr;
On 2016/12/16 12:34:40, Primiano Tucci wrote:
> let's put this back in place (see my comment on lazy polling and avoiding
> setEnabled)

Done.

https://codereview.chromium.org/2537363003/diff/320001/base/trace_event/memor...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/2537363003/diff/320001/base/trace_event/memor...
base/trace_event/memory_dump_manager_unittest.cc:157: ON_CALL(*this,
PollFastMemoryTotal(_))
On 2016/12/16 12:34:40, Primiano Tucci wrote:
> I think you don't need this really, by default the behavior of gmock should be
> to fail if a mocked method gets called unexpectedly

If I do not add this, it says:
GMOCK WARNING:
Uninteresting mock function call - returning directly.
    Function call: PollFastMemoryTotal(0x7f3f49211ef8)
NOTE: You can safely ignore the above warning unless this call should not
happen.  Do not suppress it by blindly adding an EXPECT_CALL() if you don't mean
to enforce the call.  See
http://code.google.com/p/googlemock/wiki/CookBook#Knowing_When_to_Expect for
details.

https://codereview.chromium.org/2537363003/diff/320001/base/trace_event/memor...
base/trace_event/memory_dump_manager_unittest.cc:784:
.WillRepeatedly(Invoke([](uint64_t* total) -> void {}));
On 2016/12/16 12:34:40, Primiano Tucci wrote:
> I think you don't need this empty lambda.
> Either you don't need the WillRepeatedly  part at all (by default it should do
> nothing).
> Or worst case if it doesn't build just do .WillRepeatedly(Return)

Done.

[Primiano Tucci (use gerrit), 2016-12-16 20:29:10]

LGTM

https://codereview.chromium.org/2537363003/diff/320001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2537363003/diff/320001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:799: if
(!subtle::NoBarrier_Load(&memory_tracing_enabled_))
On 2016/12/16 18:58:22, ssid wrote:
> On 2016/12/16 12:34:40, Primiano Tucci wrote:
> > why is this required?
> 
> Not really required, just an optimization to return faster.
> In case tracing is enabled without "memory-infra", we will still be holding 2
> locks and checking if out members are set. This can just be avoided if we
never
> enabled memory-infra.
> Added this now because extra lock is added.
Ahh I see the point now, right . Good point.

https://codereview.chromium.org/2537363003/diff/320001/base/trace_event/memor...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/2537363003/diff/320001/base/trace_event/memor...
base/trace_event/memory_dump_manager_unittest.cc:157: ON_CALL(*this,
PollFastMemoryTotal(_))
On 2016/12/16 18:58:22, ssid wrote:
> On 2016/12/16 12:34:40, Primiano Tucci wrote:
> > I think you don't need this really, by default the behavior of gmock should
be
> > to fail if a mocked method gets called unexpectedly
> 
> If I do not add this, it says:
> GMOCK WARNING:
> Uninteresting mock function call - returning directly.
>     Function call: PollFastMemoryTotal(0x7f3f49211ef8)
> NOTE: You can safely ignore the above warning unless this call should not
> happen.  Do not suppress it by blindly adding an EXPECT_CALL() if you don't
mean
> to enforce the call.  See
> http://code.google.com/p/googlemock/wiki/CookBook#Knowing_When_to_Expect for
> details.

Ah ok then. I remembered something like that but I thought it was a fatal error,
not a warning. Your way is better then

[ssid, 2016-12-18 20:40:58]

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

[commit-bot: I haz the power, 2016-12-18 20:41:03]

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-12-18 23:08:39]

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

[commit-bot: I haz the power, 2016-12-18 23:08:40]

Dry run: This issue passed the CQ dry run.

[ssid, 2016-12-19 00:47:52]

The CQ bit was checked by ssid@chromium.org

[ssid, 2016-12-19 00:47:53]

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/2537363003/#ps360001
(title: "nits.")

[commit-bot: I haz the power, 2016-12-19 00:48:03]

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-12-19 00:52:15]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1482108472852090,
"parent_rev": "44c6be5707fbaa0842e60d6778652a579fe4491a", "commit_rev":
"a420dfe484a4ae507599bed1d62e94d04db9fb9a"}

[commit-bot: I haz the power, 2016-12-19 00:52:44]

Description was changed from

==========
[memory-infra] Add support for polling memory totals in MemoryDumpManager

This adds new method in MemoryDumpProvider interface PollFastMemoryTotal
which is used for polling memory totals in process. The dump provider
has to enable this feature while registration.
For Simplicity of polling logic the dump providers supporting polling
must not have task runner affinity. Polling will be done on dump thread.

BUG=607533
==========

to

==========
[memory-infra] Add support for polling memory totals in MemoryDumpManager

This adds new method in MemoryDumpProvider interface PollFastMemoryTotal
which is used for polling memory totals in process. The dump provider
has to enable this feature while registration.
For Simplicity of polling logic the dump providers supporting polling
must not have task runner affinity. Polling will be done on dump thread.

BUG=607533

Review-Url: https://codereview.chromium.org/2537363003
==========

[commit-bot: I haz the power, 2016-12-19 00:52:45]

Committed patchset #7 (id:360001)

[commit-bot: I haz the power, 2016-12-19 00:56:26]

Description was changed from

==========
[memory-infra] Add support for polling memory totals in MemoryDumpManager

This adds new method in MemoryDumpProvider interface PollFastMemoryTotal
which is used for polling memory totals in process. The dump provider
has to enable this feature while registration.
For Simplicity of polling logic the dump providers supporting polling
must not have task runner affinity. Polling will be done on dump thread.

BUG=607533

Review-Url: https://codereview.chromium.org/2537363003
==========

to

==========
[memory-infra] Add support for polling memory totals in MemoryDumpManager

This adds new method in MemoryDumpProvider interface PollFastMemoryTotal
which is used for polling memory totals in process. The dump provider
has to enable this feature while registration.
For Simplicity of polling logic the dump providers supporting polling
must not have task runner affinity. Polling will be done on dump thread.

BUG=607533

Committed: https://crrev.com/4460b9b6393448adc011207ae63380a70ddba369
Cr-Commit-Position: refs/heads/master@{#439390}
==========

[commit-bot: I haz the power, 2016-12-19 00:56:27]

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