Purge memory before suspending timer queues when backgrounded.

Purge memory before suspending timer queues when backgrounded.

Timer queue suspension when backgrounded is implemented in render_scheduler_impl.cc, and is enabled in android platform.
Before suspending timer queue when backgrounded, it is worth purging the backgrounded renderer's memory. Because the renderer is suspended.
So this cl does:
- invoke purge (via MemoryCoordinator) before timer queue suspension,
- use SuspendRenderer() method instead of the original timer queue suspension, and
- move timer queue suspension code from renderer_scheduler_impl.cc to render_thread_impl.cc (because it seems not good to use MemoryCoordinator in blink platform).

BUG=

Review-Url: https://codereview.chromium.org/2668663002
Cr-Commit-Position: refs/heads/master@{#450596}
Committed: https://chromium.googlesource.com/chromium/src/+/99717061d509c250beba480db52a142ebf8b6ec8

[tasak, 2017-01-31 08:32:34]

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

[commit-bot: I haz the power, 2017-01-31 08:32: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, 2017-01-31 08:48:56]

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

[commit-bot: I haz the power, 2017-01-31 08:48:57]

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

[tasak, 2017-01-31 09:55:28]

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

[commit-bot: I haz the power, 2017-01-31 09:55:36]

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-01-31 12:33:37]

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

[commit-bot: I haz the power, 2017-01-31 12:33:37]

Dry run: This issue passed the CQ dry run.

[tasak, 2017-02-01 06:25:06]

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

[commit-bot: I haz the power, 2017-02-01 06:25:23]

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

[tasak, 2017-02-01 07:06:52]

Patchset #3 (id:40001) has been deleted

[tasak, 2017-02-01 07:08:03]

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

[commit-bot: I haz the power, 2017-02-01 07:08:23]

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

[tasak, 2017-02-01 08:54:26]

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

[commit-bot: I haz the power, 2017-02-01 08:54:38]

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

[tasak, 2017-02-01 10:27:15]

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

[commit-bot: I haz the power, 2017-02-01 10:27:29]

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

[tasak, 2017-02-01 10:27:47]

Patchset #4 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-02-01 12:04:16]

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

[commit-bot: I haz the power, 2017-02-01 12:04:17]

Dry run: This issue passed the CQ dry run.

[tasak, 2017-02-02 04:38:27]

Patchset #4 (id:100001) has been deleted

[tasak, 2017-02-02 04:38:36]

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

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

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

[tasak, 2017-02-02 05:30:47]

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

[tasak, 2017-02-02 05:30:48]

Would you review this CL?

[commit-bot: I haz the power, 2017-02-02 06:43:12]

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

[commit-bot: I haz the power, 2017-02-02 06:43:12]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-02-02 08:53:13]

Mostly looks good but please add more explanation to the CL description.

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

https://codereview.chromium.org/2668663002/diff/120001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1685: void
RenderThreadImpl::PurgeBeforeSuspendTimerQueue() {

PurgeBeforeSuspendTimerQueue => PurgeAndSuspendTimerQueueWhenBackgrounded

https://codereview.chromium.org/2668663002/diff/120001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1690: base::MemoryState::SUSPENDED);

You don't need to do this in this CL but we want to add a UMA to measure the
memory impact of the purging.

https://codereview.chromium.org/2668663002/diff/120001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1693: base::TimeDelta::FromSeconds(5));

Add a comment and mention why this delay is needed.

https://codereview.chromium.org/2668663002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
(left):

https://codereview.chromium.org/2668663002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:2474:
TEST_F(RendererSchedulerImplTest, TestRendererBackgroundedTimerSuspension) {

It's unfortunate that we're losing the test. Can we add the test somewhere else?

[tasak, 2017-02-03 03:12:56]

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

[tasak, 2017-02-03 03:13:14]

Thank you for review.

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

https://codereview.chromium.org/2668663002/diff/120001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1685: void
RenderThreadImpl::PurgeBeforeSuspendTimerQueue() {
On 2017/02/02 08:53:12, haraken wrote:
> 
> PurgeBeforeSuspendTimerQueue => PurgeAndSuspendTimerQueueWhenBackgrounded
> 

Done.

https://codereview.chromium.org/2668663002/diff/120001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1690: base::MemoryState::SUSPENDED);
On 2017/02/02 08:53:13, haraken wrote:
> 
> You don't need to do this in this CL but we want to add a UMA to measure the
> memory impact of the purging.

Acknowledged.

https://codereview.chromium.org/2668663002/diff/120001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1693: base::TimeDelta::FromSeconds(5));
On 2017/02/02 08:53:13, haraken wrote:
> 
> Add a comment and mention why this delay is needed.

Done.

https://codereview.chromium.org/2668663002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
(left):

https://codereview.chromium.org/2668663002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:2474:
TEST_F(RendererSchedulerImplTest, TestRendererBackgroundedTimerSuspension) {
On 2017/02/02 08:53:13, haraken wrote:
> 
> It's unfortunate that we're losing the test. Can we add the test somewhere
else?

I think, render_thread_impl_browsertest. render_thread_impl_unittest is not
RenderThreadImpl's test. It tests only RenderThreadImpl::HistogramCustomizer.

I think, SuspendRenderer() and ResumeRenderer() are tested in
renderer_scheduler's unittest. So we need to see whether Purge method is invoked
or not and whether SuspendRenderer() is invoked or not.
To do so, we need to make RendererSchedulerImpl to use
base::SimpleTestTickClock, because we don't want to wait 5 minutes until the
test is finished.

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

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

[haraken, 2017-02-03 04:30:49]

Please explain what this CL is doing in the CL description. LGTM with it.

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

https://codereview.chromium.org/2668663002/diff/140001/content/renderer/rende...
content/renderer/render_thread_impl.cc:906: base::Unretained(this)));

Can we call this just before calling PostDelayedTask?

https://codereview.chromium.org/2668663002/diff/140001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1667: base::Unretained(this)));

Ditto. Maybe can we Reset the closure just before calling PostDelayedTask?

[commit-bot: I haz the power, 2017-02-03 04:44:16]

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

[commit-bot: I haz the power, 2017-02-03 04:44:17]

Dry run: This issue passed the CQ dry run.

[tasak, 2017-02-03 05:53:09]

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

[commit-bot: I haz the power, 2017-02-03 05:53:49]

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

[tasak, 2017-02-03 05:55:27]

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

https://codereview.chromium.org/2668663002/diff/140001/content/renderer/rende...
content/renderer/render_thread_impl.cc:906: base::Unretained(this)));
On 2017/02/03 04:30:49, haraken wrote:
> 
> Can we call this just before calling PostDelayedTask?

Done.

https://codereview.chromium.org/2668663002/diff/140001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1667: base::Unretained(this)));
On 2017/02/03 04:30:49, haraken wrote:
> 
> Ditto. Maybe can we Reset the closure just before calling PostDelayedTask?
> 

Done.

[tasak, 2017-02-03 06:04:20]

Description was changed from

==========
Purge memory before suspending timer queues when backgrounded.

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

to

==========
Purge memory before suspending timer queues when backgrounded.

Timer queue suspension when backgrounded is implemented in
render_scheduler_impl.cc, and is enabled in android platform.
Before suspending timer queue when backgrounded, it is worth purging the
backgrounded renderer's memory. Because the renderer is suspended.
So this cl does:
- invoke purge (via MemoryCoordinator) before timer queue suspension,
- use SuspendRenderer() method instead of the original timer queue suspension,
and
- move timer queue suspension code from renderer_scheduler_impl.cc to
render_thread_impl.cc (because it seems not good to use MemoryCoordinator in
blink platform).

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

[commit-bot: I haz the power, 2017-02-03 07:21:42]

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

[commit-bot: I haz the power, 2017-02-03 07:21:43]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-02-03 07:41:56]

LGTM

[tasak, 2017-02-03 10:30:44]

tasak@google.com changed reviewers:
+ jdduke@chromium.org, skyostil@chromium.org

[tasak, 2017-02-03 10:30:45]

Would you review this CL?

[Sami, 2017-02-03 20:34:56]

skyostil@chromium.org changed reviewers:
- jdduke@chromium.org

[Sami, 2017-02-03 20:34:56]

Could you explain the "move timer queue suspension code from
renderer_scheduler_impl.cc" part of this change in a bit more detail? I think
the suspension functionality feels a lot like it should remain in the scheduler.
I'm not sure what the problem with using MemoryCoordinator from there is?

[tasak, 2017-02-06 05:18:01]

On 2017/02/03 20:34:56, Sami (slow -- travelling) wrote:
> Could you explain the "move timer queue suspension code from
> renderer_scheduler_impl.cc" part of this change in a bit more detail? I think
> the suspension functionality feels a lot like it should remain in the
scheduler.
> I'm not sure what the problem with using MemoryCoordinator from there is?

As far as I understand:
- MemoryCoordinator is implemented in content/ layer. blink's memory coordinator
is not the same as content/'s memory coordinator.
- third_party/WebKit/Source/platform cannot depend on content/ layer.

So firstly I think, there are basically two ways:
(1) Post purge task by using PostDelayedTask(4.5min) in
RenderThreadImpl::OnProcessBackground(true), or
(2) Move TimerQueueSuspension code from RenderSchedulerImpl to RenderThreadImpl
and invoke purge task before TimerQueueSuspension.

I think, (1) is too hacky solution... So I tried (2).

[Sami, 2017-02-06 12:03:17]

On 2017/02/06 05:18:01, tasak wrote:
> As far as I understand:
> - MemoryCoordinator is implemented in content/ layer. blink's memory
coordinator
> is not the same as content/'s memory coordinator.
> - third_party/WebKit/Source/platform cannot depend on content/ layer.
> 
> So firstly I think, there are basically two ways:
> (1) Post purge task by using PostDelayedTask(4.5min) in
> RenderThreadImpl::OnProcessBackground(true), or
> (2) Move TimerQueueSuspension code from RenderSchedulerImpl to
RenderThreadImpl
> and invoke purge task before TimerQueueSuspension.
> 
> I think, (1) is too hacky solution... So I tried (2).

I see. Could we find a way to let the Blink scheduler call into the memory
coordinator when purging needs to happen? For example we could add a suspension
observer to WebScheduler, or something kind of pure virtual
WebScheduler::MemoryCoordinator interface. WDYT?

I still think timer queue suspension is pretty clearly a scheduler feature so
the code for that should remain there.

[haraken, 2017-02-07 06:56:28]

On 2017/02/03 20:34:56, Sami (slow -- travelling) wrote:
> Could you explain the "move timer queue suspension code from
> renderer_scheduler_impl.cc" part of this change in a bit more detail? I think
> the suspension functionality feels a lot like it should remain in the
scheduler.
> I'm not sure what the problem with using MemoryCoordinator from there is?

The reason is (just) that we didn't want to introduce a public API to connect
Blink Scheduler and content/. If we have the suspension logic in content/, no
public API is needed (as the CL shows).

I don't have a strong opinion on where the suspension logic should be placed,
but ideally I think it should be placed in MemoryCoordinator in the browser
process since MemoryCoordinator knows what tabs can be suspended etc. However,
MemoryCoordinator is not yet available, so we need to place it in either Blink
Scheduler or content/. Either is fine -- that's my understanding.

If you prefer putting it in Blink Scheduler, tasak@ will add a public API.

[Sami, 2017-02-07 12:54:14]

On 2017/02/07 06:56:28, haraken wrote:
> The reason is (just) that we didn't want to introduce a public API to connect
> Blink Scheduler and content/. If we have the suspension logic in content/, no
> public API is needed (as the CL shows).
> 
> I don't have a strong opinion on where the suspension logic should be placed,
> but ideally I think it should be placed in MemoryCoordinator in the browser
> process since MemoryCoordinator knows what tabs can be suspended etc. However,
> MemoryCoordinator is not yet available, so we need to place it in either Blink
> Scheduler or content/. Either is fine -- that's my understanding.
> 
> If you prefer putting it in Blink Scheduler, tasak@ will add a public API.

Got it, thanks for the explanation. Sorry to be stubborn but I think a small
public API is a better option than adding more stuff to RenderThreadImpl. For
instance, I notice we're deleting a test here but not adding any because
RenderThreadImpl is pretty hard to test in isolation :)

[tasak, 2017-02-08 09:59:00]

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

[commit-bot: I haz the power, 2017-02-08 09:59:24]

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

[tasak, 2017-02-08 10:13:56]

On 2017/02/07 12:54:14, Sami wrote:
> On 2017/02/07 06:56:28, haraken wrote:
> > The reason is (just) that we didn't want to introduce a public API to
connect
> > Blink Scheduler and content/. If we have the suspension logic in content/,
no
> > public API is needed (as the CL shows).
> > 
> > I don't have a strong opinion on where the suspension logic should be
placed,
> > but ideally I think it should be placed in MemoryCoordinator in the browser
> > process since MemoryCoordinator knows what tabs can be suspended etc.
However,
> > MemoryCoordinator is not yet available, so we need to place it in either
Blink
> > Scheduler or content/. Either is fine -- that's my understanding.
> > 
> > If you prefer putting it in Blink Scheduler, tasak@ will add a public API.
> 
> Got it, thanks for the explanation. Sorry to be stubborn but I think a small
> public API is a better option than adding more stuff to RenderThreadImpl. For
> instance, I notice we're deleting a test here but not adding any because
> RenderThreadImpl is pretty hard to test in isolation :)

I see. I added requestPurgeMemory to blink::Platform and made
RendererSchedulerImpl to invoke the method when background timer queue
suspension.

[tasak, 2017-02-08 10:14:09]

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

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

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

[commit-bot: I haz the power, 2017-02-08 11:23:12]

Dry run: This issue passed the CQ dry run.

[Sami, 2017-02-08 16:57:19]

Thank you! lgtm.

[bashi, 2017-02-09 07:34:58]

lgtm

https://codereview.chromium.org/2668663002/diff/180001/content/renderer/rende...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/2668663002/diff/180001/content/renderer/rende...
content/renderer/renderer_blink_platform_impl.cc:1300: void
RendererBlinkPlatformImpl::requestPurgeMemory() {
nit: DCHECK(memory_coordinator_) ?

https://codereview.chromium.org/2668663002/diff/180001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/Platform.h (right):

https://codereview.chromium.org/2668663002/diff/180001/third_party/WebKit/pub...
third_party/WebKit/public/platform/Platform.h:672: // Memory
------------------------------------------------------------
nit: Could you add a description?

// Requests purging memory. The platform may or may not purge memory, depending
on memory pressure.

[tasak, 2017-02-10 10:20:48]

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

[commit-bot: I haz the power, 2017-02-10 10:21: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, 2017-02-10 11:44:02]

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

[commit-bot: I haz the power, 2017-02-10 11:44:04]

Dry run: This issue passed the CQ dry run.

[tasak, 2017-02-14 07:28:10]

Thank you for review.

https://codereview.chromium.org/2668663002/diff/180001/content/renderer/rende...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/2668663002/diff/180001/content/renderer/rende...
content/renderer/renderer_blink_platform_impl.cc:1300: void
RendererBlinkPlatformImpl::requestPurgeMemory() {
On 2017/02/09 07:34:57, bashi wrote:
> nit: DCHECK(memory_coordinator_) ?

Done.

https://codereview.chromium.org/2668663002/diff/180001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/Platform.h (right):

https://codereview.chromium.org/2668663002/diff/180001/third_party/WebKit/pub...
third_party/WebKit/public/platform/Platform.h:672: // Memory
------------------------------------------------------------
On 2017/02/09 07:34:58, bashi wrote:
> nit: Could you add a description?
> 
> // Requests purging memory. The platform may or may not purge memory,
depending
> on memory pressure.

Done.

[haraken, 2017-02-14 07:34:09]

LGTM

[tasak, 2017-02-14 07:45:52]

tasak@google.com changed reviewers:
+ avi@chromium.org

[tasak, 2017-02-14 07:45:53]

avi@, would you review this CL?

I need content/ owner's lgtm.

[Avi (use Gerrit), 2017-02-14 15:52:16]

lgtm

stampity stamp

[tasak, 2017-02-15 04:30:59]

On 2017/02/14 15:52:16, Avi wrote:
> lgtm
> 
> stampity stamp

Thank you.

[tasak, 2017-02-15 04:59:19]

The CQ bit was checked by tasak@google.com

[tasak, 2017-02-15 04:59:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org, bashi@chromium.org, haraken@chromium.org,
avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2668663002/#ps220001
(title: "Patch for landing")

[commit-bot: I haz the power, 2017-02-15 05:00:06]

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-02-15 06:34:10]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1487134759282460,
"parent_rev": "f01f0b358c7733ee91e7c017430300786a47f043", "commit_rev":
"99717061d509c250beba480db52a142ebf8b6ec8"}

[commit-bot: I haz the power, 2017-02-15 06:35:00]

Description was changed from

==========
Purge memory before suspending timer queues when backgrounded.

Timer queue suspension when backgrounded is implemented in
render_scheduler_impl.cc, and is enabled in android platform.
Before suspending timer queue when backgrounded, it is worth purging the
backgrounded renderer's memory. Because the renderer is suspended.
So this cl does:
- invoke purge (via MemoryCoordinator) before timer queue suspension,
- use SuspendRenderer() method instead of the original timer queue suspension,
and
- move timer queue suspension code from renderer_scheduler_impl.cc to
render_thread_impl.cc (because it seems not good to use MemoryCoordinator in
blink platform).

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

to

==========
Purge memory before suspending timer queues when backgrounded.

Timer queue suspension when backgrounded is implemented in
render_scheduler_impl.cc, and is enabled in android platform.
Before suspending timer queue when backgrounded, it is worth purging the
backgrounded renderer's memory. Because the renderer is suspended.
So this cl does:
- invoke purge (via MemoryCoordinator) before timer queue suspension,
- use SuspendRenderer() method instead of the original timer queue suspension,
and
- move timer queue suspension code from renderer_scheduler_impl.cc to
render_thread_impl.cc (because it seems not good to use MemoryCoordinator in
blink platform).

BUG=

Review-Url: https://codereview.chromium.org/2668663002
Cr-Commit-Position: refs/heads/master@{#450596}
Committed:
https://chromium.googlesource.com/chromium/src/+/99717061d509c250beba480db52a...
==========

[commit-bot: I haz the power, 2017-02-15 06:35:02]

Committed patchset #9 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/99717061d509c250beba480db52a...