Set DISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY to true on Android.

Set DISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY to true on Android.

Also change render_process_visibility_manager to use
MEMORY_PRESSURE_MODERATE when the renderer loses visibility
since MEMORY_PRESSURE_CRITICAL should be reserved for
situations where memory pressure is really critical.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211635

[Ian Vollick, 2013-07-10 01:33:37]

On 2013/07/09 15:29:41, Ross Mcilroy wrote:

Offline, you'd suggested using the name SYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE
instead. I like it, and I think it's appropriate here.

That said, I think we also need the DISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY
define (and it should be defined on android).

The SYSTEM_NATIVE.. should let us turn on code to produce memory pressure
notifications when the system won't provide them (eg, in
RenderProcessVisibilityManager), and DISCARDABLE_MEMORY_AL... will be used in an
upcoming CL (https://codereview.chromium.org/17106004/) to conditionally compile
code for emulating discardable memory.

I've conflated the two concepts, but as you pointed out, abusing
DISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY as a hint for turning on artificial
memory notifications doesn't make sense.

Since the plumbing will be so similar, can you please add support for both in
this patch?

[rmcilroy, 2013-07-10 13:22:48]

On 2013/07/10 01:33:37, vollick wrote:
> On 2013/07/09 15:29:41, Ross Mcilroy wrote:
> 
> Offline, you'd suggested using the name
SYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE
> instead. I like it, and I think it's appropriate here.
> 
> That said, I think we also need the
DISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY
> define (and it should be defined on android).
> 
> The SYSTEM_NATIVE.. should let us turn on code to produce memory pressure
> notifications when the system won't provide them (eg, in
> RenderProcessVisibilityManager), and DISCARDABLE_MEMORY_AL... will be used in
an
> upcoming CL (https://codereview.chromium.org/17106004/) to conditionally
compile
> code for emulating discardable memory.
> 
> I've conflated the two concepts, but as you pointed out, abusing
> DISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY as a hint for turning on
artificial
> memory notifications doesn't make sense.
> 
> Since the plumbing will be so similar, can you please add support for both in
> this patch?

Done.  Also took the liberty of changing MEMORY_PRESSURE_CRITICAL to
MEMORY_PRESSURE_MODERATE while I was in that file given the offline thread.

[Ian Vollick, 2013-07-10 14:51:53]

On 2013/07/10 13:22:48, Ross Mcilroy wrote:
> On 2013/07/10 01:33:37, vollick wrote:
> > On 2013/07/09 15:29:41, Ross Mcilroy wrote:
> > 
> > Offline, you'd suggested using the name
> SYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE
> > instead. I like it, and I think it's appropriate here.
> > 
> > That said, I think we also need the
> DISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY
> > define (and it should be defined on android).
> > 
> > The SYSTEM_NATIVE.. should let us turn on code to produce memory pressure
> > notifications when the system won't provide them (eg, in
> > RenderProcessVisibilityManager), and DISCARDABLE_MEMORY_AL... will be used
in
> an
> > upcoming CL (https://codereview.chromium.org/17106004/) to conditionally
> compile
> > code for emulating discardable memory.
> > 
> > I've conflated the two concepts, but as you pointed out, abusing
> > DISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY as a hint for turning on
> artificial
> > memory notifications doesn't make sense.
> > 
> > Since the plumbing will be so similar, can you please add support for both
in
> > this patch?
> 
> Done.  Also took the liberty of changing MEMORY_PRESSURE_CRITICAL to
> MEMORY_PRESSURE_MODERATE while I was in that file given the offline thread.

lgtm. Thanks!

[commit-bot: I haz the power, 2013-07-10 14:55:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/18910002/4001

[commit-bot: I haz the power, 2013-07-10 15:11:29]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[rmcilroy, 2013-07-10 15:28:50]

James: could you approve for content/renderer OWNERS please.

[jamesr, 2013-07-10 20:28:21]

https://codereview.chromium.org/18910002/diff/4001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/18910002/diff/4001/build/common.gypi#newcode421
build/common.gypi:421: # Set to true if platform natively supports discarable
memory.
typo "discarable" -> "discardable".

Saying "Set to true if ..." is redundant

https://codereview.chromium.org/18910002/diff/4001/build/common.gypi#newcode423
build/common.gypi:423: # Set to true if platform sends memory pressure signals
natively.
blank line before this

https://codereview.chromium.org/18910002/diff/4001/content/renderer/render_pr...
File content/renderer/render_process_visibility_manager.cc (right):

https://codereview.chromium.org/18910002/diff/4001/content/renderer/render_pr...
content/renderer/render_process_visibility_manager.cc:32:
base::MemoryPressureListener::MEMORY_PRESSURE_MODERATE);
this looks like a change in behavior. why?

[rmcilroy, 2013-07-11 08:57:27]

Thanks for the comments James.  They should be addressed in the latest patchset.

https://codereview.chromium.org/18910002/diff/4001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/18910002/diff/4001/build/common.gypi#newcode421
build/common.gypi:421: # Set to true if platform natively supports discarable
memory.
On 2013/07/10 20:28:22, jamesr wrote:
> typo "discarable" -> "discardable".
> 
> Saying "Set to true if ..." is redundant

Done.

https://codereview.chromium.org/18910002/diff/4001/build/common.gypi#newcode423
build/common.gypi:423: # Set to true if platform sends memory pressure signals
natively.
On 2013/07/10 20:28:22, jamesr wrote:
> blank line before this

Done.

https://codereview.chromium.org/18910002/diff/4001/content/renderer/render_pr...
File content/renderer/render_process_visibility_manager.cc (right):

https://codereview.chromium.org/18910002/diff/4001/content/renderer/render_pr...
content/renderer/render_process_visibility_manager.cc:32:
base::MemoryPressureListener::MEMORY_PRESSURE_MODERATE);
On 2013/07/10 20:28:22, jamesr wrote:
> this looks like a change in behavior. why?

Yes, this is an intended behavior change.  I've looped you into the thread where
Ian and I discussed this, but the reason to change this call's argument is
because the MEMORY_PRESSURE_CRITICAL notification is very heavyweight (intended
for when Android is just about to kill Chrome due to lack of memory), so we do
things like emergency GCs in V8, which is not necessarily appropriate for
backgrounding a tab.  MEMORY_PRESSURE_MODERATE is more appropriate here since
that is the  notification we send on Android when a renderer process is
backgrounded (via a system notification).

[rmcilroy, 2013-07-11 09:07:38]

https://codereview.chromium.org/18910002/diff/4001/content/renderer/render_pr...
File content/renderer/render_process_visibility_manager.cc (right):

https://codereview.chromium.org/18910002/diff/4001/content/renderer/render_pr...
content/renderer/render_process_visibility_manager.cc:32:
base::MemoryPressureListener::MEMORY_PRESSURE_MODERATE);
On 2013/07/11 08:57:27, Ross Mcilroy wrote:
> On 2013/07/10 20:28:22, jamesr wrote:
> > this looks like a change in behavior. why?
> 
> Yes, this is an intended behavior change.  I've looped you into the thread
where
> Ian and I discussed this, but the reason to change this call's argument is
> because the MEMORY_PRESSURE_CRITICAL notification is very heavyweight
(intended
> for when Android is just about to kill Chrome due to lack of memory), so we do
> things like emergency GCs in V8, which is not necessarily appropriate for
> backgrounding a tab.  MEMORY_PRESSURE_MODERATE is more appropriate here since
> that is the  notification we send on Android when a renderer process is
> backgrounded (via a system notification).

I also updated the CL's comment to include this change in behavior.

[jamesr, 2013-07-13 01:41:29]

lgtm

[commit-bot: I haz the power, 2013-07-15 08:36:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/18910002/16001

[commit-bot: I haz the power, 2013-07-15 08:43:09]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-07-15 12:05:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/18910002/16001

[commit-bot: I haz the power, 2013-07-15 13:28:52]

Change committed as 211635