[memory-infra] MemoryDumpScheduler follows config for time between dumps

[memory-infra] MemoryDumpScheduler follows config for time between dumps

The scheduler should reset the time since the last dump on all processes
when every global dump happens. So, MemoryDumpManager should notify the
scheduler when dumps are triggered.
Use enum instead of multiple bool to store the state of polling.

BUG=607533
Review-Url: https://codereview.chromium.org/2736283003
Cr-Commit-Position: refs/heads/master@{#456268}
Committed: https://chromium.googlesource.com/chromium/src/+/5a15b1b1fa8f202b16250541522d22a070b03e1f

[ssid, 2017-03-09 04:13:29]

Description was changed from

==========
Clean up MemoryDumpScheduler

Use enum instead of multiple bool.

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

to

==========
[memory-infra] MemoryDumpScheduler follows config for time between dumps

The scheduler should reset the time since the last dump on all processes
when every global dump happens. So, MemoryDumpManager should notify the
scheduler when dumps are triggered.
Use enum instead of multiple bool to store the state of polling.

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

[ssid, 2017-03-09 04:13:38]

Patchset #1 (id:1) has been deleted

[ssid, 2017-03-09 18:26:50]

Patchset #1 (id:20001) has been deleted

[ssid, 2017-03-09 18:26:58]

Patchset #1 (id:40001) has been deleted

[ssid, 2017-03-09 18:28:18]

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

[ssid, 2017-03-09 18:28:18]

+primiano wdyt?

[ssid, 2017-03-09 18:28:59]

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

[commit-bot: I haz the power, 2017-03-09 18:29:22]

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-03-09 19:30:14]

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

[commit-bot: I haz the power, 2017-03-09 19:30:15]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2017-03-10 10:35:39]

The code itself LG, I just realized that we might have some problem with threads
& posttask due to the lifetime of the scheduler, which is not indefinite. see
comments below

https://codereview.chromium.org/2736283003/diff/80001/base/trace_event/memory...
File base/trace_event/memory_dump_scheduler.cc (right):

https://codereview.chromium.org/2736283003/diff/80001/base/trace_event/memory...
base/trace_event/memory_dump_scheduler.cc:107: ThreadTaskRunnerHandle::Get() !=
polling_state_.polling_task_runner) {
do you really want to compare the TaskRunnerHandles? That should be a pretty
rare need.
by looking at the code it seems to be you just want to use:

polling_state_.polling_task_runner.RunsTasksOnCurrentThread()

In general you should not rely on the fact that two calls to
ThreadTaskRunnerHandle::Get() o the same thread returns the same handle. task
runner handles are kind of proxy objects. 

Sorry I just realized that there is a similar instance below and I didn't spot
in the previous review.

https://codereview.chromium.org/2736283003/diff/80001/base/trace_event/memory...
base/trace_event/memory_dump_scheduler.cc:110:
Bind(&MemoryDumpScheduler::NotifyDumpTriggered, Unretained(this)));
Also I just realized that these Unretained aren't really safe. The MDScheduler
is reset OnTraceLogDisabled, so these post task might end up causing
Use-After-Free. I think you might need a weakptr here, or figure out some better
ownership semantic.

[ssid, 2017-03-10 14:57:37]

https://codereview.chromium.org/2736283003/diff/80001/base/trace_event/memory...
File base/trace_event/memory_dump_scheduler.cc (right):

https://codereview.chromium.org/2736283003/diff/80001/base/trace_event/memory...
base/trace_event/memory_dump_scheduler.cc:110:
Bind(&MemoryDumpScheduler::NotifyDumpTriggered, Unretained(this)));
On 2017/03/10 10:35:39, Primiano Tucci (slow - perf) wrote:
> Also I just realized that these Unretained aren't really safe. The MDScheduler
> is reset OnTraceLogDisabled, so these post task might end up causing
> Use-After-Free. I think you might need a weakptr here, or figure out some
better
> ownership semantic.

The reason why the unretained should be safe here is because of the thread.Stop
called by MDM before destroying the scheduler. There could an issue with the
task runner itself being null while posting, but that will again be handled by
MDM which takes lock while accessing scheduler, and does not call it after trace
log disabled takes away the scheduler. Only after the |dump_schefuler| is null
we call DisableTriggers which then makes the task runner null.

[Primiano Tucci (use gerrit), 2017-03-10 15:08:36]

Hmm ok let's do this:  This LGTM on the assumptions you wrote here, otherwise we
keep piling up too many things.

Honestly I'd be happier if we could figure out some stronger pattern to avoid
the risk of technical debt.
I am out of bandwidth this week to think. So I'll try to think to this problem
next week. In the meantime let's not be blocked.

[ssid, 2017-03-10 23:48:23]

Patchset #3 (id:100001) has been deleted

[ssid, 2017-03-10 23:50:21]

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

[commit-bot: I haz the power, 2017-03-10 23:50:57]

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

[ssid, 2017-03-10 23:51:10]

I am working on trying to make the scheduler singleton after this patch

https://codereview.chromium.org/2736283003/diff/80001/base/trace_event/memory...
File base/trace_event/memory_dump_scheduler.cc (right):

https://codereview.chromium.org/2736283003/diff/80001/base/trace_event/memory...
base/trace_event/memory_dump_scheduler.cc:107: ThreadTaskRunnerHandle::Get() !=
polling_state_.polling_task_runner) {
On 2017/03/10 10:35:39, Primiano Tucci (slow - perf) wrote:
> do you really want to compare the TaskRunnerHandles? That should be a pretty
> rare need.
> by looking at the code it seems to be you just want to use:
> 
> polling_state_.polling_task_runner.RunsTasksOnCurrentThread()
> 
> In general you should not rely on the fact that two calls to
> ThreadTaskRunnerHandle::Get() o the same thread returns the same handle. task
> runner handles are kind of proxy objects. 
> 
> Sorry I just realized that there is a similar instance below and I didn't spot
> in the previous review.

Yes my bad. Fixed it.

[commit-bot: I haz the power, 2017-03-11 01:20:40]

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

[commit-bot: I haz the power, 2017-03-11 01:20:41]

Dry run: This issue passed the CQ dry run.

[ssid, 2017-03-11 03:01:57]

The CQ bit was checked by ssid@chromium.org

[ssid, 2017-03-11 03:01:57]

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/2736283003/#ps120001
(title: "fix task runner check and rebase.")

[commit-bot: I haz the power, 2017-03-11 03:02:11]

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-03-11 03:07:41]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489201317251770,
"parent_rev": "b31f5f8032bb90935408fcf41aeca3fde1ca0f1c", "commit_rev":
"5a15b1b1fa8f202b16250541522d22a070b03e1f"}

[commit-bot: I haz the power, 2017-03-11 03:08:19]

Description was changed from

==========
[memory-infra] MemoryDumpScheduler follows config for time between dumps

The scheduler should reset the time since the last dump on all processes
when every global dump happens. So, MemoryDumpManager should notify the
scheduler when dumps are triggered.
Use enum instead of multiple bool to store the state of polling.

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

to

==========
[memory-infra] MemoryDumpScheduler follows config for time between dumps

The scheduler should reset the time since the last dump on all processes
when every global dump happens. So, MemoryDumpManager should notify the
scheduler when dumps are triggered.
Use enum instead of multiple bool to store the state of polling.

BUG=607533

Review-Url: https://codereview.chromium.org/2736283003
Cr-Commit-Position: refs/heads/master@{#456268}
Committed:
https://chromium.googlesource.com/chromium/src/+/5a15b1b1fa8f202b16250541522d...
==========

[commit-bot: I haz the power, 2017-03-11 03:08:21]

Committed patchset #3 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/5a15b1b1fa8f202b16250541522d...