791263 - [ARMv6] completely disable screenshot code (including memory allocation) on memory pressure

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Right now if ScreenshotHandler.getInstance() is called even once it will allocate and hold on to a large memory buffer for screenshotting. It would be good to ensure that this buffer gets freed if screenshotting is disabled, and also to make screenshotting controlled by a pref rather than a hard-coded value.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Disable screenshotting on low memory and free the buffer

I decided to just make it get disabled dynamically on low memory instead of adding a pref.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 661903 [details] [diff] [review]
Disable screenshotting on low memory and free the buffer

Review of attachment 661903 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoApp.java
@@ +2130,5 @@
>      {
>          Log.e(LOGTAG, "low memory");
>          if (checkLaunchState(LaunchState.GeckoRunning))
>              GeckoAppShell.onLowMemory();
> +        ScreenshotHandler.disableScreenshot();

should we turn it back on at some point? What if this low memory condition is temporary (heavy page, lots of tabs, sync happening, etc)?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Maybe. I wasn't really sure if we should because if it's a heavy page or lots of tabs then that might just be the usage pattern of the user. If that's the case we're likely to run into low-mem situations often and it's better to just leave it off.

If you want though I could try to add a check for memory usage when we close a tab, and if we have a bunch of free memory we can re-enable screenshotting.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

I'm thinking that once we get into this "disable things to lower memory usage" we should set a timer to query the memory situation periodically and see if we can start turning things back on.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 661903 [details] [diff] [review]
Disable screenshotting on low memory and free the buffer

Sounds reasonable. I'll cook up a patch with that.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Disable screenshotting on low memory and free the buffer (v2)

This applies on top of the MemoryMonitor patch from bug 713032 which is on inbound now. I don't want to land this just yet, I'd rather get bug 792165 resolved first so hopefully we can measure the improvement this makes. Putting this patch here anyway as a working patch modulo future rebasing.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 662687 [details] [diff] [review]
Disable screenshotting on low memory and free the buffer (v2)

Review of attachment 662687 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/MemoryMonitor.java
@@ +149,5 @@
>      }
>  
>      private synchronized boolean decreaseMemoryPressure() {
> +        int newLevel;
> +        synchronized (this) {

this is redundant, the method is synchronized, which is equivalent to synchronized (this) aiui

Comment 8

[Kartikaya Gupta (email:[e-mail])]

(In reply to Brad Lassey [:blassey] from comment #7)
> >      private synchronized boolean decreaseMemoryPressure() {
> > +        int newLevel;
> > +        synchronized (this) {
> 
> this is redundant, the method is synchronized, which is equivalent to
> synchronized (this) aiui

Oh whoops, I meant to remove the synchronization on the method and just leave the smaller block synchronized. I don't want to run external code while holding on to the MemoryMonitor lock.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Landed with that synchronization taken out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5898731aa944

Comment 10

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/5898731aa944

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

changing summary to reflect what this patch did