Resume a backgrounded renderer that was purged and suspended

Resume a backgrounded renderer that was purged and suspended

- To avoid breaking web, we need to resume a backgrounded renderer that was purged and suspended.
(A backgrounded renderer is suspended for 120seconds).
After resumed, the renderer is running for 10 seconds and is purged and suspended again.
- intent-to-implement-and-ship of background renderer's purge + suspend is
 https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
- one of the documents attached in the above intent is
 https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJvI/edit?usp=sharing
- The feature is not enabled by default because purge-and-suspend-time is 0.

BUG=635419

[tasak, 2016-09-30 09:55:59]

Patchset #1 (id:1) has been deleted

[tasak, 2016-09-30 09:56:08]

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

[commit-bot: I haz the power, 2016-09-30 09:56: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-09-30 10:11:23]

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

[commit-bot: I haz the power, 2016-09-30 10:11:24]

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

[tasak, 2016-09-30 10:26:54]

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

[commit-bot: I haz the power, 2016-09-30 10:27:02]

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

[tasak, 2016-09-30 10:27:18]

Patchset #1 (id:20001) has been deleted

[tasak, 2016-09-30 10:33:56]

Description was changed from

==========
Resume a backgrounded suspended renderer.

BUG=635419
==========

to

==========
Resume a backgrounded renderer that was purged and suspended

To avoid breaking web, we need to resume a backgrounded renderer that was purged
and suspended. After resumed, the renderer is running for a while and is purged
and suspended again.


BUG=635419
==========

[tasak, 2016-09-30 10:34:07]

Description was changed from

==========
Resume a backgrounded renderer that was purged and suspended

To avoid breaking web, we need to resume a backgrounded renderer that was purged
and suspended. After resumed, the renderer is running for a while and is purged
and suspended again.


BUG=635419
==========

to

==========
Resume a backgrounded renderer that was purged and suspended

To avoid breaking web, we need to resume a backgrounded renderer that was purged
and suspended.
After resumed, the renderer is running for a while and is purged and suspended
again.


BUG=635419
==========

[tasak, 2016-09-30 10:40:50]

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

[tasak, 2016-09-30 10:40:51]

Would you review this CL?

[hajimehoshi, 2016-09-30 11:22:46]

I'm not clear about the description of this CL. Are you changing the
purge+suspended tab to wake up periodically?

https://codereview.chromium.org/2387603003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2387603003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:997:
last_resumed_time_ = last_purged_and_suspended_time_;
Is this updated when the tab is foregrounded and resumed?

https://codereview.chromium.org/2387603003/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h
(right):

https://codereview.chromium.org/2387603003/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h:135:
// Tells the scheduler that the render process should be resumeed. This can
s/resumeed/resumed/

[commit-bot: I haz the power, 2016-09-30 11:45:06]

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

[commit-bot: I haz the power, 2016-09-30 11:45:07]

Dry run: This issue passed the CQ dry run.

[Sami, 2016-09-30 12:10:39]

skyostil@chromium.org changed reviewers:
+ altimin@chromium.org, skyostil@chromium.org

[Sami, 2016-09-30 12:10:40]

It feels like we should try to unify this approach (and purge & suspend in
general) with budget-based renderer throttling[1] somehow. Maybe we could only
de-purge every once in a while if the web page actually tries to do something
periodically. Did you have some specific reason for choosing 2 minutes every 20
minutes?

[1]
https://docs.google.com/document/d/1vCUeGfr2xzZ67SFt2yZjNeaIcXGp2Td6KHN7bI02y...

[haraken, 2016-10-03 04:26:37]

On 2016/09/30 12:10:40, Sami wrote:
> It feels like we should try to unify this approach (and purge & suspend in
> general) with budget-based renderer throttling[1] somehow.

In long term, yes.

That said, at least in short term we need the purge+suspend because it's
important that timer and loading task runners *completely* stop for a while. If
we run any JavaScript task, it can resume a lot of memory.

> Maybe we could only
> de-purge every once in a while if the web page actually tries to do something
> periodically.

We discussed this before, but there is no easy way to detect if the webpage does
something that needs resuming. For example, the webpage may receive a push
notification after 10 mins after the page load. To make it work, we need to keep
repeating purge+suspend periodically.

> Did you have some specific reason for choosing 2 minutes every 20
> minutes?

No strong reason for the 2 minutes.

> 
> [1]
>
https://docs.google.com/document/d/1vCUeGfr2xzZ67SFt2yZjNeaIcXGp2Td6KHN7bI02y...

[tasak, 2016-10-03 05:43:21]

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

[commit-bot: I haz the power, 2016-10-03 05:43:36]

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

[tasak, 2016-10-03 05:44:16]

https://codereview.chromium.org/2387603003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2387603003/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:997:
last_resumed_time_ = last_purged_and_suspended_time_;
On 2016/09/30 11:22:46, hajimehoshi wrote:
> Is this updated when the tab is foregrounded and resumed?

No. This is only updated when the tab is backgrounded and resumed.
When the tab is foregrounded, the last active time is updated.

https://codereview.chromium.org/2387603003/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h
(right):

https://codereview.chromium.org/2387603003/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h:135:
// Tells the scheduler that the render process should be resumeed. This can
On 2016/09/30 11:22:46, hajimehoshi wrote:
> s/resumeed/resumed/

Done.

[commit-bot: I haz the power, 2016-10-03 07:09:06]

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

[commit-bot: I haz the power, 2016-10-03 07:09:07]

Dry run: This issue passed the CQ dry run.

[tasak, 2016-10-04 09:18:07]

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

[tasak, 2016-10-06 05:18:28]

Would you review this CL?

Currently I have no idea about how to measure user experience when changing the
duration between suspending and resuming (i.e. the 20min and 2min).
So if it is possible to introduce some metrics, I would like to tune the
duration.

[haraken, 2016-10-06 07:29:05]

https://codereview.chromium.org/2387603003/diff/60001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2387603003/diff/60001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:89: const int
kResumeSuspendedRendererMinutes = 20;

I'd say 20 seconds would be too short.

https://codereview.chromium.org/2387603003/diff/60001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:91: // After resuming and running a
backgrounded rendererr for this period,

renderer

https://codereview.chromium.org/2387603003/diff/60001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:93: const int
kSuspendResumedRendererMinutes = 2;

Would it be hard to run the renderer until it has drained all pending tasks?

[tasak, 2016-10-06 07:59:49]

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

[commit-bot: I haz the power, 2016-10-06 08:00:06]

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

[tasak, 2016-10-06 08:02:06]

Thank you for review.

https://codereview.chromium.org/2387603003/diff/60001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2387603003/diff/60001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:89: const int
kResumeSuspendedRendererMinutes = 20;
On 2016/10/06 07:29:05, haraken wrote:
> 
> I'd say 20 seconds would be too short.

2min.
Done.

https://codereview.chromium.org/2387603003/diff/60001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:91: // After resuming and running a
backgrounded rendererr for this period,
On 2016/10/06 07:29:05, haraken wrote:
> 
> renderer

Done.

https://codereview.chromium.org/2387603003/diff/60001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:93: const int
kSuspendResumedRendererMinutes = 2;
On 2016/10/06 07:29:05, haraken wrote:
> 
> Would it be hard to run the renderer until it has drained all pending tasks?

I think, it is difficult to ask task runners in tab_manager.

[haraken, 2016-10-06 08:21:20]

LGTM on my side, but Sami should take a look.

- Add a link to the Intent-to-implement to the CL description.

- Also mention that the this CL does not enable purge + suspend. Our plan is to
enable the feature only on a Finch experiment.

- In long term, the suspension mechanism should be unified with the budget-based
throttling of background renderers controlled by a global coordinator. It will
take a lot of time to get there, so I'm fine with introducing the suspension
mechanism in this CL but if anyone has a concern, I'm open for ideas :)

https://codereview.chromium.org/2387603003/diff/80001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2387603003/diff/80001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:93: const int
kSuspendResumedRendererMinutes = 1;

As discussed offline, 1 min => 10 sec?

[tasak, 2016-10-06 08:27:43]

Description was changed from

==========
Resume a backgrounded renderer that was purged and suspended

To avoid breaking web, we need to resume a backgrounded renderer that was purged
and suspended.
After resumed, the renderer is running for a while and is purged and suspended
again.


BUG=635419
==========

to

==========
Resume a backgrounded renderer that was purged and suspended

- To avoid breaking web, we need to resume a backgrounded renderer that was
purged and suspended. (A backgrounded renderer is suspended for 120seconds).
After resumed, the renderer is running for 10 seconds and is purged and
suspended again.
- intent-to-implement-and-ship of background renderer's purge + suspend is
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
- one of the documents attached in the above intent is
https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ...
- The feature is not enabled by default because purge-and-suspend-time is 0.

BUG=635419
==========

[tasak, 2016-10-06 08:27:58]

Description was changed from

==========
Resume a backgrounded renderer that was purged and suspended

- To avoid breaking web, we need to resume a backgrounded renderer that was
purged and suspended. (A backgrounded renderer is suspended for 120seconds).
After resumed, the renderer is running for 10 seconds and is purged and
suspended again.
- intent-to-implement-and-ship of background renderer's purge + suspend is
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
- one of the documents attached in the above intent is
https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ...
- The feature is not enabled by default because purge-and-suspend-time is 0.

BUG=635419
==========

to

==========
Resume a backgrounded renderer that was purged and suspended

- To avoid breaking web, we need to resume a backgrounded renderer that was
purged and suspended.
(A backgrounded renderer is suspended for 120seconds).
After resumed, the renderer is running for 10 seconds and is purged and
suspended again.
- intent-to-implement-and-ship of background renderer's purge + suspend is
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
- one of the documents attached in the above intent is
https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ...
- The feature is not enabled by default because purge-and-suspend-time is 0.

BUG=635419
==========

[tasak, 2016-10-06 08:28:25]

Description was changed from

==========
Resume a backgrounded renderer that was purged and suspended

- To avoid breaking web, we need to resume a backgrounded renderer that was
purged and suspended.
(A backgrounded renderer is suspended for 120seconds).
After resumed, the renderer is running for 10 seconds and is purged and
suspended again.
- intent-to-implement-and-ship of background renderer's purge + suspend is
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
- one of the documents attached in the above intent is
https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ...
- The feature is not enabled by default because purge-and-suspend-time is 0.

BUG=635419
==========

to

==========
Resume a backgrounded renderer that was purged and suspended

- To avoid breaking web, we need to resume a backgrounded renderer that was
purged and suspended.
(A backgrounded renderer is suspended for 120seconds).
After resumed, the renderer is running for 10 seconds and is purged and
suspended again.
- intent-to-implement-and-ship of background renderer's purge + suspend is
 https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
- one of the documents attached in the above intent is

https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ...
- The feature is not enabled by default because purge-and-suspend-time is 0.

BUG=635419
==========

[tasak, 2016-10-06 08:32:38]

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

[commit-bot: I haz the power, 2016-10-06 08:32:49]

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

[tasak, 2016-10-06 08:42:37]

> - Add a link to the Intent-to-implement to the CL description.

Done.

> - Also mention that the this CL does not enable purge + suspend. Our plan is
to
> enable the feature only on a Finch experiment.

Done.

https://codereview.chromium.org/2387603003/diff/80001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2387603003/diff/80001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:93: const int
kSuspendResumedRendererMinutes = 1;
On 2016/10/06 08:21:20, haraken wrote:
> 
> As discussed offline, 1 min => 10 sec?

Done.

[dcheng, 2016-10-06 09:55:08]

driveby

https://codereview.chromium.org/2387603003/diff/80001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2387603003/diff/80001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:93: const int
kSuspendResumedRendererMinutes = 1;
On 2016/10/06 08:42:36, tasak wrote:
> On 2016/10/06 08:21:20, haraken wrote:
> > 
> > As discussed offline, 1 min => 10 sec?
> 
> Done.

Nit: this can just be a constexpr base::TimeDelta. Then it can just be used
directly below without having to construct a TimeDelta. The name can be shorter
too because it doesn't need to include the units anymore =)

[tasak, 2016-10-06 10:09:03]

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

[commit-bot: I haz the power, 2016-10-06 10:09:21]

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

[tasak, 2016-10-06 10:10:05]

https://codereview.chromium.org/2387603003/diff/80001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2387603003/diff/80001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:93: const int
kSuspendResumedRendererMinutes = 1;
On 2016/10/06 09:55:08, dcheng wrote:
> On 2016/10/06 08:42:36, tasak wrote:
> > On 2016/10/06 08:21:20, haraken wrote:
> > > 
> > > As discussed offline, 1 min => 10 sec?
> > 
> > Done.
> 
> Nit: this can just be a constexpr base::TimeDelta. Then it can just be used
> directly below without having to construct a TimeDelta. The name can be
shorter
> too because it doesn't need to include the units anymore =)

I see.
Done.
Thank you.

[commit-bot: I haz the power, 2016-10-06 12:42:11]

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

[commit-bot: I haz the power, 2016-10-06 12:42:12]

Dry run: This issue passed the CQ dry run.

[Sami, 2016-10-06 13:10:09]

lgtm with a test :)

By the way, one possible way to improve this in the future would be to suspend
individual WebViews instead of waiting for entire renderers to become
backgrounded. The renderer scheduler can already mostly do that.

https://codereview.chromium.org/2387603003/diff/120001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2387603003/diff/120001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:87: // If a backgrounded renderer is kept
backgrounded during this time after
s/during/for/
/after suspended/after being suspended/

?

https://codereview.chromium.org/2387603003/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
(right):

https://codereview.chromium.org/2387603003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:513:
void RendererSchedulerImpl::ResumeRenderer() {
Could you please add a test for this in RendererSchedulerImplTest?

[Sami, 2016-10-06 13:11:53]

Btw #2, a suspended renderer also means TaskQueueThrottler won't keep ticking,
right altimin@?

[chrisha, 2016-10-06 13:18:45]

I'm curious why we're putting this in TabManager instead of in
MemoryCoordinator?

[tasak, 2016-10-07 05:30:49]

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

[commit-bot: I haz the power, 2016-10-07 05:31:15]

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

[tasak, 2016-10-07 05:31:26]

Thank you for review.

https://codereview.chromium.org/2387603003/diff/120001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2387603003/diff/120001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:87: // If a backgrounded renderer is kept
backgrounded during this time after
On 2016/10/06 13:10:09, Sami wrote:
> s/during/for/
> /after suspended/after being suspended/
> 
> ?

Done.

https://codereview.chromium.org/2387603003/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
(right):

https://codereview.chromium.org/2387603003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:513:
void RendererSchedulerImpl::ResumeRenderer() {
On 2016/10/06 13:10:09, Sami wrote:
> Could you please add a test for this in RendererSchedulerImplTest?

Done.

[tasak, 2016-10-07 05:40:29]

On 2016/10/06 13:18:45, chrisha (slow) wrote:
> I'm curious why we're putting this in TabManager instead of in
> MemoryCoordinator?

Yeah, we are planning to move purge&suspend code to MemoryCoorinator.
So firstly bashi@ created a patch to invoke Purge&Suspend via MemoryCoordinator:
https://codereview.chromium.org/2376233005/

So is it better to wait until bashi@'s patch is landed?

[commit-bot: I haz the power, 2016-10-07 06:53:53]

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

[commit-bot: I haz the power, 2016-10-07 06:53:54]

Dry run: This issue passed the CQ dry run.

[altimin, 2016-10-07 16:16:14]

On 2016/10/06 13:11:53, Sami wrote:
> Btw #2, a suspended renderer also means TaskQueueThrottler won't keep ticking,
> right altimin@?

Yes and no — currently |pump_throttled_tasks_closure_| is not cancelled and it
will produce unnecessary wakeup. I suggest to do following to fix that:
1. Add a method to force update next task pumping (something like
TaskQueueThrottler::ForceUpdateNextPumpThrottledTasks) and call it when renderer
is suspended or resumed.
2. In this method |pump_throttled_tasks_closure_| needs to be cancelled and new
pump time calculated (logic should be similar to one in PumpThrottledTasks,
maybe it's possible to refactor it).


Separate note on time-based throttling and purge-and-suspend:
Currently if both flags are enabled, both limitations take place at the same
time. That means that:
1. For every two minutes renderer will be suspended followed by 10 seconds of
activity.
2. Time-based limit of 1% will continue to apply to timer queues. That means
that time budget will continue to accumulate when renderer is suspended, and out
of 10 seconds of activity timers are allowed to run for 1% * 2 minutes = 1.2
seconds of time or they be throttled. (Personally I would like to consider this
a feature, preventing different renderers from consuming 100% of CPU in turns.
If it doesn't work for you options are: to disable time-budget throttling when
purge-and-suspend are active or add additional time budget when resuming
renderer)

[haraken, 2016-10-08 02:16:48]

On 2016/10/07 16:16:14, altimin wrote:
> On 2016/10/06 13:11:53, Sami wrote:
> > Btw #2, a suspended renderer also means TaskQueueThrottler won't keep
ticking,
> > right altimin@?
> 
> Yes and no — currently |pump_throttled_tasks_closure_| is not cancelled and it
> will produce unnecessary wakeup. I suggest to do following to fix that:
> 1. Add a method to force update next task pumping (something like
> TaskQueueThrottler::ForceUpdateNextPumpThrottledTasks) and call it when
renderer
> is suspended or resumed.
> 2. In this method |pump_throttled_tasks_closure_| needs to be cancelled and
new
> pump time calculated (logic should be similar to one in PumpThrottledTasks,
> maybe it's possible to refactor it).
> 
> 
> Separate note on time-based throttling and purge-and-suspend:
> Currently if both flags are enabled, both limitations take place at the same
> time. That means that:
> 1. For every two minutes renderer will be suspended followed by 10 seconds of
> activity.
> 2. Time-based limit of 1% will continue to apply to timer queues. That means
> that time budget will continue to accumulate when renderer is suspended, and
out
> of 10 seconds of activity timers are allowed to run for 1% * 2 minutes = 1.2
> seconds of time or they be throttled. (Personally I would like to consider
this
> a feature, preventing different renderers from consuming 100% of CPU in turns.
> If it doesn't work for you options are: to disable time-budget throttling when
> purge-and-suspend are active or add additional time budget when resuming
> renderer)

Sami will be in Tokyo next week, so I'll chat with him how to unify the
timer-budget scheduling with the tab suspension.

[tasak, 2016-10-12 07:01:41]

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

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

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-10-12 09:15:48]

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

[commit-bot: I haz the power, 2016-10-12 09:15:49]

Dry run: This issue passed the CQ dry run.

[tasak, 2016-10-18 03:04:43]

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

[commit-bot: I haz the power, 2016-10-18 03:05:02]

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

[tasak, 2016-10-18 05:17:49]

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

[tasak, 2016-10-18 05:17:50]

avi, would you review this CL?

[commit-bot: I haz the power, 2016-10-18 05:29:15]

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

[commit-bot: I haz the power, 2016-10-18 05:29:16]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-10-18 06:15:37]

https://codereview.chromium.org/2387603003/diff/80001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2387603003/diff/80001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:93: const int
kSuspendResumedRendererMinutes = 1;
On 2016/10/06 09:55:08, dcheng wrote:
> On 2016/10/06 08:42:36, tasak wrote:
> > On 2016/10/06 08:21:20, haraken wrote:
> > > 
> > > As discussed offline, 1 min => 10 sec?
> > 
> > Done.
> 
> Nit: this can just be a constexpr base::TimeDelta. Then it can just be used
> directly below without having to construct a TimeDelta. The name can be
shorter
> too because it doesn't need to include the units anymore =)

dcheng: Are you sure that making a constexpr TimeDelta here doesn't create a
static initializer?

https://codereview.chromium.org/2387603003/diff/200001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2387603003/diff/200001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:95: base::TimeDelta::FromSeconds(120);
These comments seem to be opposite what you say in the CL description.

The CL description says
- Renderer suspended for 120s
- Renderer resumed for 10s
repeat

whereas these comments read to me that
"if a [...] renderer is kept backgrounded for [...10s...] resume it" and
"after [...] running a backgrounded renderer for [...120s...] then suspend it
again"

which seems the opposite, and also seems less sensical. (And seems in
disagreement with their use in code below.)

Also, these names are so abbreviated and similar to be confusable, and make no
sense out of context. Can you find them new names?
kMaxTimeRendererAllowedToBeSuspendedBeforeResume and
kSuspendedRendererLengthOfResumption as examples of what come to mind.

https://codereview.chromium.org/2387603003/diff/200001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:737: ((current_time - last_resumed_time) >
kSuspendResumedRendererTime))) {
Holy moly. I just spent twenty minutes pondering this if() and I'm still not
sure I understand it or that it's correct.

My first thought is that this is

if (A || (B && C && D))

A and C are opposites. If A is true, we short-circuit (B && C && D). If A is
false, C is true, so we shouldn't need it.

Otherwise...
ugh...

We have three times here:
1. last_purged_and_suspended_time
2. last_resumed_time
3. tab.last_active

plus a boolean of whether it's been 10s since last_resumed_time.

That's six orderings of time ×2 for the boolean = 12 possibilities. Which ones
are the ones that trigger this? I can't tell. Can you lay out the possibilities
in a comment, so that as a code reader I can convince myself that it's correct?

https://codereview.chromium.org/2387603003/diff/200001/content/public/browser...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/2387603003/diff/200001/content/public/browser...
content/public/browser/render_process_host.h:326: // Gets the last time when the
renderer process is purged and suspended.
was purged...

https://codereview.chromium.org/2387603003/diff/200001/content/public/browser...
content/public/browser/render_process_host.h:329: // Gets the last time when the
renderer process is resumed in the background.
was resumed...

[tasak, 2016-10-18 10:30:42]

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

[commit-bot: I haz the power, 2016-10-18 10:30:48]

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

[tasak, 2016-10-18 10:38:42]

Thank you for review.

https://codereview.chromium.org/2387603003/diff/200001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2387603003/diff/200001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:95: base::TimeDelta::FromSeconds(120);
On 2016/10/18 06:15:37, Avi wrote:
> These comments seem to be opposite what you say in the CL description.
> 
> The CL description says
> - Renderer suspended for 120s
> - Renderer resumed for 10s
> repeat
> 
> whereas these comments read to me that
> "if a [...] renderer is kept backgrounded for [...10s...] resume it" and
> "after [...] running a backgrounded renderer for [...120s...] then suspend it
> again"
> 
> which seems the opposite, and also seems less sensical. (And seems in
> disagreement with their use in code below.)
> 
> Also, these names are so abbreviated and similar to be confusable, and make no
> sense out of context. Can you find them new names?
> kMaxTimeRendererAllowedToBeSuspendedBeforeResume and
> kSuspendedRendererLengthOfResumption as examples of what come to mind.

Yeah, my comment was completely wrong. Thank you.
Done.

https://codereview.chromium.org/2387603003/diff/200001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:737: ((current_time - last_resumed_time) >
kSuspendResumedRendererTime))) {
On 2016/10/18 06:15:36, Avi wrote:
> Holy moly. I just spent twenty minutes pondering this if() and I'm still not
> sure I understand it or that it's correct.
> 
> My first thought is that this is
> 
> if (A || (B && C && D))
> 
> A and C are opposites. If A is true, we short-circuit (B && C && D). If A is
> false, C is true, so we shouldn't need it.
> 
> Otherwise...
> ugh...
> 
> We have three times here:
> 1. last_purged_and_suspended_time
> 2. last_resumed_time
> 3. tab.last_active
> 
> plus a boolean of whether it's been 10s since last_resumed_time.
> 
> That's six orderings of time ×2 for the boolean = 12 possibilities. Which ones
> are the ones that trigger this? I can't tell. Can you lay out the
possibilities
> in a comment, so that as a code reader I can convince myself that it's
correct?
> 
> 

Yeah... the code was dirty...

So I add "purge-and-suspend state" and "last modified time" to
TabManager::WebContentsData.

[commit-bot: I haz the power, 2016-10-18 11:50:07]

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

[commit-bot: I haz the power, 2016-10-18 11:50:09]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-10-18 15:41:53]

Excellent simplification. Some nits.

https://codereview.chromium.org/2387603003/diff/220001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2387603003/diff/220001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:711: PurgeAndSuspendState& next_state) {
Non-const reference arguments are not allowed.
https://google.github.io/styleguide/cppguide.html#Reference_Arguments

This is an out parameter for ShouldUpdatePurgeAndSuspendState, so it should be a
pointer.

https://codereview.chromium.org/2387603003/diff/220001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:747: }
Turning this into state transitions makes this much clearer. Thank you!

https://codereview.chromium.org/2387603003/diff/220001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:793: }
So much simpler. 👍

[tasak, 2016-10-19 04:22:49]

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

[commit-bot: I haz the power, 2016-10-19 04:23:46]

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

[tasak, 2016-10-19 06:13:41]

Thank you for review.

https://codereview.chromium.org/2387603003/diff/220001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2387603003/diff/220001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:747: }
On 2016/10/18 15:41:53, Avi wrote:
> Turning this into state transitions makes this much clearer. Thank you!

Done.

[haraken, 2016-10-19 08:57:54]

LGTM, but Chris should take a look.

What happens if a renderer is sent to foreground before the PurgeAndSuspend
signal reaches the renderer (because of a timing issue)? Then won't it happen
that the browser thinks that the renderer is suspended while the renderer is not
suspended?

https://codereview.chromium.org/2387603003/diff/240001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2387603003/diff/240001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:724: state = BACKGROUNDED;

Don't we need to set last_modified_time and WebConentsData's
last_modifiled_time_ to tab.last_active?

https://codereview.chromium.org/2387603003/diff/240001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:787: task_runner_ =
base::ThreadTaskRunnerHandle::Get();

What is this doing?

https://codereview.chromium.org/2387603003/diff/240001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager_web_contents_data.cc (right):

https://codereview.chromium.org/2387603003/diff/240001/chrome/browser/memory/...
chrome/browser/memory/tab_manager_web_contents_data.cc:27:
purge_and_suspend_state_(BACKGROUNDED) {}

Is it okay to unconditionally set the initial value to BACKGROUNDED (even though
the WebContentsData is foregrounded)?

[tasak, 2016-10-19 09:23:28]

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

[commit-bot: I haz the power, 2016-10-19 09:30:26]

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

[tasak, 2016-10-19 10:15:57]

Patchset #12 (id:260001) has been deleted

[tasak, 2016-10-19 10:16:05]

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

[commit-bot: I haz the power, 2016-10-19 10:17:02]

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

[tasak, 2016-10-19 10:17:13]

> What happens if a renderer is sent to foreground before the PurgeAndSuspend
signal reaches the renderer (because of a timing issue)? Then won't it happen
that the browser thinks that the renderer is suspended while the renderer is not
suspended?

I modified RenderThreadImpl::OnProcessPurgeAndSuspend to see RendererIsHidden().

https://codereview.chromium.org/2387603003/diff/240001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2387603003/diff/240001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:724: state = BACKGROUNDED;
On 2016/10/19 08:57:54, haraken wrote:
> 
> Don't we need to set last_modified_time and WebConentsData's
> last_modifiled_time_ to tab.last_active?

tab.last_active is updated only when the tab is shown.
So do you mean we should update tab.last_active when purge-and-suspend state is
changed?
I'm afraid that the change causes some regression of TabDiscarding.

https://codereview.chromium.org/2387603003/diff/240001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:787: task_runner_ =
base::ThreadTaskRunnerHandle::Get();
On 2016/10/19 08:57:54, haraken wrote:
> 
> What is this doing?

Removed.

https://codereview.chromium.org/2387603003/diff/240001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager_web_contents_data.cc (right):

https://codereview.chromium.org/2387603003/diff/240001/chrome/browser/memory/...
chrome/browser/memory/tab_manager_web_contents_data.cc:27:
purge_and_suspend_state_(BACKGROUNDED) {}
On 2016/10/19 08:57:54, haraken wrote:
> 
> Is it okay to unconditionally set the initial value to BACKGROUNDED (even
though
> the WebContentsData is foregrounded)?

In PurgeAndSuspendBackgroundedTabs, firstly we check whether a tab is
backgrounded or not.
So we don't need to consider "foregrounded".

[haraken, 2016-10-19 10:40:30]

https://codereview.chromium.org/2387603003/diff/280001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2387603003/diff/280001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:723: if (tab.last_active >
last_modified_time)

Instead of adding this if check, would it be possible to update
WebContentsData::purge_and_suspend_state_ to RUNNING when the tab gets
foregrounded? Then you don't need to add the if check.

I want to make sure that the following invariant holds:

- WebContentsData::purge_and_suspend_state_ == SUSPENDED iff the tab is
suspended
- WebContentsData::purge_and_suspend_state_ == RESUMED iff the tab is resumed
- WebContentsData::purge_and_suspend_state_ == RUNNING otherwise

https://codereview.chromium.org/2387603003/diff/280001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager.h (right):

https://codereview.chromium.org/2387603003/diff/280001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.h:254: enum PurgeAndSuspendState {

Add a comment about how the state transition happens.

https://codereview.chromium.org/2387603003/diff/280001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.h:255: BACKGROUNDED,

I'd prefer renaming BACKGROUNDED to RUNNING (or something), because the tab may
be foregrounded.

[commit-bot: I haz the power, 2016-10-19 12:39:26]

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

[commit-bot: I haz the power, 2016-10-19 12:39:27]

Dry run: This issue passed the CQ dry run.

[tasak, 2016-10-20 11:27:31]

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

[commit-bot: I haz the power, 2016-10-20 11:27:41]

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

[tasak, 2016-10-20 11:28:47]

https://codereview.chromium.org/2387603003/diff/280001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2387603003/diff/280001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:723: if (tab.last_active >
last_modified_time)
On 2016/10/19 10:40:30, haraken wrote:
> 
> Instead of adding this if check, would it be possible to update
> WebContentsData::purge_and_suspend_state_ to RUNNING when the tab gets
> foregrounded? Then you don't need to add the if check.

I think, it is very difficult to capture such transitions, i.e.
foregrounded/backgrounded here.
So if we move the state from here to WebContents, it is possible.
But I don't want to make the state public, because the state will be removed (or
moved) when MemoryCoordinator is landed.

https://codereview.chromium.org/2387603003/diff/280001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager.h (right):

https://codereview.chromium.org/2387603003/diff/280001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.h:255: BACKGROUNDED,
On 2016/10/19 10:40:30, haraken wrote:
> 
> I'd prefer renaming BACKGROUNDED to RUNNING (or something), because the tab
may
> be foregrounded.

Done.

[altimin, 2016-10-20 11:39:20]

https://codereview.chromium.org/2387603003/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
(right):

https://codereview.chromium.org/2387603003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:487:
void RendererSchedulerImpl::SuspendRenderer() {
I'd like to remind you that there is scheduled call to
TaskQueueThrottler::PumpThrottledTasks inside TaskQueueThrottler
(CancelableClosureHolder pump_throttled_tasks_closure_) which will wake up this
renderer. To prevent this I recommend adding a method
TaskQueueThrottler::UpdateNextThrottledTasksPump and call it after disabling all
queues.

[tasak, 2016-10-20 11:47:36]

https://codereview.chromium.org/2387603003/diff/280001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2387603003/diff/280001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:723: if (tab.last_active >
last_modified_time)
On 2016/10/20 11:28:46, tasak wrote:
> On 2016/10/19 10:40:30, haraken wrote:
> > 
> > Instead of adding this if check, would it be possible to update
> > WebContentsData::purge_and_suspend_state_ to RUNNING when the tab gets
> > foregrounded? Then you don't need to add the if check.
> 
> I think, it is very difficult to capture such transitions, i.e.
> foregrounded/backgrounded here.
> So if we move the state from here to WebContents, it is possible.
> But I don't want to make the state public, because the state will be removed
(or
> moved) when MemoryCoordinator is landed.

I found that it is possible to implement  TabManager::ActiveTabChanged.

[tasak, 2016-10-20 12:32:43]

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

[commit-bot: I haz the power, 2016-10-20 12:32:56]

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

[tasak, 2016-10-20 12:35:12]

https://codereview.chromium.org/2387603003/diff/280001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2387603003/diff/280001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:723: if (tab.last_active >
last_modified_time)
On 2016/10/20 11:47:36, tasak wrote:
> On 2016/10/20 11:28:46, tasak wrote:
> > On 2016/10/19 10:40:30, haraken wrote:
> > > 
> > > Instead of adding this if check, would it be possible to update
> > > WebContentsData::purge_and_suspend_state_ to RUNNING when the tab gets
> > > foregrounded? Then you don't need to add the if check.
> > 
> > I think, it is very difficult to capture such transitions, i.e.
> > foregrounded/backgrounded here.
> > So if we move the state from here to WebContents, it is possible.
> > But I don't want to make the state public, because the state will be removed
> (or
> > moved) when MemoryCoordinator is landed.
> 
> I found that it is possible to implement  TabManager::ActiveTabChanged.
> 

Done.

https://codereview.chromium.org/2387603003/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
(right):

https://codereview.chromium.org/2387603003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:487:
void RendererSchedulerImpl::SuspendRenderer() {
On 2016/10/20 11:39:20, altimin wrote:
> I'd like to remind you that there is scheduled call to
> TaskQueueThrottler::PumpThrottledTasks inside TaskQueueThrottler
> (CancelableClosureHolder pump_throttled_tasks_closure_) which will wake up
this
> renderer. To prevent this I recommend adding a method
> TaskQueueThrottler::UpdateNextThrottledTasksPump and call it after disabling
all
> queues.

Thank you. So I would like to update SuspendRenderer() in another CL.

[commit-bot: I haz the power, 2016-10-20 14:12:22]

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

[commit-bot: I haz the power, 2016-10-20 14:12:23]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[haraken, 2016-10-20 16:52:16]

PS14 LGTM

Chris: Would you mind double-checking if this CL is going in a right direction?

https://codereview.chromium.org/2387603003/diff/320001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2387603003/diff/320001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:88: // before resume.

A suspended renderer is suspended for this duration.

https://codereview.chromium.org/2387603003/diff/320001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:89: constexpr base::TimeDelta
kMaxTimeRendererAllowedToBeSuspendedBeforeResume =

kDurationOfRendererSuspension ?

https://codereview.chromium.org/2387603003/diff/320001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:93: // for this duration.

A resumed renderer is resumed for this duration.

https://codereview.chromium.org/2387603003/diff/320001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:94: constexpr base::TimeDelta
kSuspendedRendererLengthOfResumption =

kDurationOfRendererResumption ?

https://codereview.chromium.org/2387603003/diff/320001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:722: DCHECK(state == RUNNING ||
tab.last_active < last_modified_time);

Add a comment about what 'tab.last_active < last_modified_time' means.

https://codereview.chromium.org/2387603003/diff/320001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.cc:776: if
(!ShouldUpdatePurgeAndSuspendState(current_time, tab,

How about just making this method return the next state? Then we can compare
next_state with current_state.

https://codereview.chromium.org/2387603003/diff/320001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager.h (right):

https://codereview.chromium.org/2387603003/diff/320001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.h:269: // - When ActiveTabChaged, the newly
activted tab's state will be RUNNING.

activated

[chrisha, 2016-10-20 21:09:15]

Looks like this is going in the right direction. I'd prefer to see this broken
into several smaller CLs:

- create Resume() functionality inside renderer components.
- expose Resume() on RenderProcessHost.
- Add new fields to WebContentsData.
- Add logic to TabDiscarding.

Would make for shorter, self-contained reviews.

A hearty +1 to haraken's comments.