Provide renderers with memory pressure signals.

Provide renderers with memory pressure signals.

This uses TabManager's renderer ranking and existing memory pressure IPC mechanisms to notify renderer processes (least important ones first) of memory pressure events.

BUG=483964
Committed: https://crrev.com/8706e5a3fab86048ebbf579a8c5dbabe937aa746
Cr-Commit-Position: refs/heads/master@{#372487}

[chrisha, 2016-01-27 20:31:13]

chrisha@chromium.org changed reviewers:
+ georgesak@chromium.org

[chrisha, 2016-01-27 20:31:19]

I've left behind some comments about refactoring work to do. We can talk more
offline.

PTAL?

[Georges Khalil, 2016-01-28 15:41:02]

Awesome! Minor comments only.

And yes, refactoring is greatly needed.

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

https://codereview.chromium.org/1641813002/diff/20001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:770: CHECK(!renderers.empty());
Could this be triggered while shutting down and no more renderers are alive?

If so, maybe add a check that the browser is not shutting down, otherwise do
nothing.

https://codereview.chromium.org/1641813002/diff/20001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager.h (right):

https://codereview.chromium.org/1641813002/diff/20001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.h:123:
FRIEND_TEST_ALL_PREFIXES(TabManagerTest, ChildProcessNotifications);
nit: alphabetical order.

https://codereview.chromium.org/1641813002/diff/20001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.h:129: enum : int {
kRendererNotificationDelayInSeconds = 2 };
Add a comment explaining where that value comes from.

[chrisha, 2016-01-28 22:23:03]

Description was changed from

==========
Provide renderers with memory pressure signals.

This uses TabManager's renderer ranking and existing memory pressure IPC
mechanisms to notify renderer processes (least important ones first) of memory
pressure events.

BUG=483964
==========

to

==========
Provide renderers with memory pressure signals.

This uses TabManager's renderer ranking and existing memory pressure IPC
mechanisms to notify renderer processes (least important ones first) of memory
pressure events.

BUG=483964
==========

[chrisha, 2016-01-28 22:23:03]

chrisha@chromium.org changed reviewers:
+ creis@chromium.org

[chrisha, 2016-01-28 22:23:25]

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

[commit-bot: I haz the power, 2016-01-28 22:24:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1641813002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1641813002/40001

[commit-bot: I haz the power, 2016-01-28 22:28:24]

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

[commit-bot: I haz the power, 2016-01-28 22:28:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chrisha, 2016-01-28 22:40:26]

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

[commit-bot: I haz the power, 2016-01-28 22:42:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1641813002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1641813002/60001

[commit-bot: I haz the power, 2016-01-28 22:55:10]

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

[commit-bot: I haz the power, 2016-01-28 22:55:11]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chrisha, 2016-01-29 15:21:12]

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

[commit-bot: I haz the power, 2016-01-29 15:22:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1641813002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1641813002/80001

[chrisha, 2016-01-29 16:17:54]

Merged https://codereview.chromium.org/1640423002/ with this CL.

creis: PTAL at everything in content/
georgesak: Another look at things in chrome/browser/memory?

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

https://codereview.chromium.org/1641813002/diff/20001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:770: CHECK(!renderers.empty());
On 2016/01/28 15:41:02, Georges Khalil wrote:
> Could this be triggered while shutting down and no more renderers are alive?
> 
> If so, maybe add a check that the browser is not shutting down, otherwise do
> nothing.

Very good point, as that's a possible race condition.

https://codereview.chromium.org/1641813002/diff/20001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager.h (right):

https://codereview.chromium.org/1641813002/diff/20001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.h:123:
FRIEND_TEST_ALL_PREFIXES(TabManagerTest, ChildProcessNotifications);
On 2016/01/28 15:41:02, Georges Khalil wrote:
> nit: alphabetical order.

Done.

https://codereview.chromium.org/1641813002/diff/20001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.h:129: enum : int {
kRendererNotificationDelayInSeconds = 2 };
On 2016/01/28 15:41:02, Georges Khalil wrote:
> Add a comment explaining where that value comes from.

Done.

[Charlie Reis, 2016-01-29 19:46:56]

Just a few comment nits on content/.  Otherwise looks good!

https://codereview.chromium.org/1641813002/diff/100001/content/browser/memory...
File content/browser/memory/memory_pressure_controller_impl.h (right):

https://codereview.chromium.org/1641813002/diff/100001/content/browser/memory...
content/browser/memory/memory_pressure_controller_impl.h:26: // messages to be
forward to individual processes. This allows the browser
nit: forwarded

https://codereview.chromium.org/1641813002/diff/100001/content/browser/memory...
content/browser/memory/memory_pressure_controller_impl.h:30: class
CONTENT_EXPORT MemoryPressureControllerImpl {
It'd be more common to have this derive from MemoryPressureController and
override the public methods, but I can see that doesn't work as well with your
singleton approach.  This should be fine.

https://codereview.chromium.org/1641813002/diff/100001/content/browser/memory...
content/browser/memory/memory_pressure_controller_impl.h:62: static
MemoryPressureControllerImpl* GetInstance();
nit: Move this to be the first public method.

https://codereview.chromium.org/1641813002/diff/100001/content/public/browser...
File content/public/browser/memory_pressure_controller.h (right):

https://codereview.chromium.org/1641813002/diff/100001/content/public/browser...
content/public/browser/memory_pressure_controller.h:16: // Facade around the
singleton MemoryPressureControllerImpl class defined in
We probably shouldn't introduce a public class in terms of its impl class, since
people working on content embedders shouldn't have to dig through our
implementations.

I'm ok leaving a reference to the impl.h file for more details, but maybe the
rest of the comment can be phrased in a more general way?

https://codereview.chromium.org/1641813002/diff/100001/content/public/browser...
content/public/browser/memory_pressure_controller.h:18: // more details. These
are exposed so that chrome/browser can interact with the
Let's say "content embedders" rather than chrome/browser.

https://codereview.chromium.org/1641813002/diff/100001/content/public/browser...
content/public/browser/memory_pressure_controller.h:22: static void
SendPressureNotification(
Let's copy the comments on these functions here, so that people don't have to
read the impl file unless they want to understand how it works.

https://codereview.chromium.org/1641813002/diff/100001/content/public/browser...
content/public/browser/memory_pressure_controller.h:28: private:
nit: Add blank line above.

https://codereview.chromium.org/1641813002/diff/100001/content/public/browser...
content/public/browser/memory_pressure_controller.h:34: #endif  //
CONTENT_PUBLIC_BROWSER_MEMORY_MEMORY_PRESSURE_CONTROLLER_H_
Looks like you have an extra MEMORY_ here.

[chrisha, 2016-01-29 21:12:13]

Thanks creis@. Comments inline. Another look?

https://codereview.chromium.org/1641813002/diff/100001/content/browser/memory...
File content/browser/memory/memory_pressure_controller_impl.h (right):

https://codereview.chromium.org/1641813002/diff/100001/content/browser/memory...
content/browser/memory/memory_pressure_controller_impl.h:26: // messages to be
forward to individual processes. This allows the browser
On 2016/01/29 19:46:56, Charlie Reis wrote:
> nit: forwarded

Done.

https://codereview.chromium.org/1641813002/diff/100001/content/browser/memory...
content/browser/memory/memory_pressure_controller_impl.h:30: class
CONTENT_EXPORT MemoryPressureControllerImpl {
On 2016/01/29 19:46:56, Charlie Reis wrote:
> It'd be more common to have this derive from MemoryPressureController and
> override the public methods, but I can see that doesn't work as well with your
> singleton approach.  This should be fine.

Yeah... I wasn't 100% sure whether or not this was the way to go.

https://codereview.chromium.org/1641813002/diff/100001/content/browser/memory...
content/browser/memory/memory_pressure_controller_impl.h:62: static
MemoryPressureControllerImpl* GetInstance();
On 2016/01/29 19:46:56, Charlie Reis wrote:
> nit: Move this to be the first public method.

Done.

https://codereview.chromium.org/1641813002/diff/100001/content/public/browser...
File content/public/browser/memory_pressure_controller.h (right):

https://codereview.chromium.org/1641813002/diff/100001/content/public/browser...
content/public/browser/memory_pressure_controller.h:16: // Facade around the
singleton MemoryPressureControllerImpl class defined in
On 2016/01/29 19:46:56, Charlie Reis wrote:
> We probably shouldn't introduce a public class in terms of its impl class,
since
> people working on content embedders shouldn't have to dig through our
> implementations.
> 
> I'm ok leaving a reference to the impl.h file for more details, but maybe the
> rest of the comment can be phrased in a more general way?

Done.

https://codereview.chromium.org/1641813002/diff/100001/content/public/browser...
content/public/browser/memory_pressure_controller.h:18: // more details. These
are exposed so that chrome/browser can interact with the
On 2016/01/29 19:46:56, Charlie Reis wrote:
> Let's say "content embedders" rather than chrome/browser.

Done.

https://codereview.chromium.org/1641813002/diff/100001/content/public/browser...
content/public/browser/memory_pressure_controller.h:22: static void
SendPressureNotification(
On 2016/01/29 19:46:56, Charlie Reis wrote:
> Let's copy the comments on these functions here, so that people don't have to
> read the impl file unless they want to understand how it works.

Done.

https://codereview.chromium.org/1641813002/diff/100001/content/public/browser...
content/public/browser/memory_pressure_controller.h:28: private:
On 2016/01/29 19:46:56, Charlie Reis wrote:
> nit: Add blank line above.

Done.

https://codereview.chromium.org/1641813002/diff/100001/content/public/browser...
content/public/browser/memory_pressure_controller.h:34: #endif  //
CONTENT_PUBLIC_BROWSER_MEMORY_MEMORY_PRESSURE_CONTROLLER_H_
On 2016/01/29 19:46:56, Charlie Reis wrote:
> Looks like you have an extra MEMORY_ here.

Indeed I do. Removed.

[chrisha, 2016-01-29 21:13:11]

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

[commit-bot: I haz the power, 2016-01-29 21:15:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1641813002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1641813002/120001

[Charlie Reis, 2016-01-29 21:29:37]

Thanks, content/ LGTM.

[commit-bot: I haz the power, 2016-01-29 22:45:46]

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

[commit-bot: I haz the power, 2016-01-29 22:45:47]

Dry run: This issue passed the CQ dry run.

[chrisha, 2016-01-30 00:04:33]

The CQ bit was checked by chrisha@chromium.org

[commit-bot: I haz the power, 2016-01-30 00:06:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1641813002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1641813002/120001

[commit-bot: I haz the power, 2016-01-30 00:14:20]

Description was changed from

==========
Provide renderers with memory pressure signals.

This uses TabManager's renderer ranking and existing memory pressure IPC
mechanisms to notify renderer processes (least important ones first) of memory
pressure events.

BUG=483964
==========

to

==========
Provide renderers with memory pressure signals.

This uses TabManager's renderer ranking and existing memory pressure IPC
mechanisms to notify renderer processes (least important ones first) of memory
pressure events.

BUG=483964
==========

[commit-bot: I haz the power, 2016-01-30 00:14:21]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-01-30 00:16:13]

Description was changed from

==========
Provide renderers with memory pressure signals.

This uses TabManager's renderer ranking and existing memory pressure IPC
mechanisms to notify renderer processes (least important ones first) of memory
pressure events.

BUG=483964
==========

to

==========
Provide renderers with memory pressure signals.

This uses TabManager's renderer ranking and existing memory pressure IPC
mechanisms to notify renderer processes (least important ones first) of memory
pressure events.

BUG=483964

Committed: https://crrev.com/8706e5a3fab86048ebbf579a8c5dbabe937aa746
Cr-Commit-Position: refs/heads/master@{#372487}
==========

[commit-bot: I haz the power, 2016-01-30 00:16:13]

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