962963 - Be more aggressive about expiring tabs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

As seen in https://areweslimyet.com/mobile, tabs can hold onto a good bit of memory. We have mechanisms to help reduce the active tab count and reclaim some memory. One way is through tab expiration:

Expire (zombify) tabs after a given time period (3600 seconds)
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#8294

Looking at recent telemetry, tab expiration typically happens way longer than 3600 secs. The 25th percentile is over 4500 secs. 75th percentile is over 26000 secs. This is probably due to the way we fire the expiration code, which only happens when adding a new tab.

We could lower the expiration time and we could use a timer to trigger expiration.

Comment 1

[Richard Newman [:rnewman]]

I was thinking about whether we can do this more efficiently, in response to memory measurements.

For historical context, see Bug 744515 Comment 4, and Bug 744515 Comment 9, in which kats suggests using a SoftReference as a memory pressure detector.

For modern situational knowledge, read <http://developer.android.com/training/articles/memory.html#ReleaseMemoryAsTight>. Most of the contents of that document are ICS-onward, which is increasingly less of a problem.

I contend that we should be discarding tabs based on onTrimMemory calls; see MemoryMonitor.java. We already drop the FaviconCache, and (as far as I can tell) we invoke nsMemoryPressure's NS_DispatchMemoryPressure function.

The right thing to do here is probably to:

* Hook Java-side stuff into MemoryMonitor.
* Attach Gecko-side stuff to the MemPressure stuff emitted by nsMemoryPressure.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #1)

> I contend that we should be discarding tabs based on onTrimMemory calls; see
> MemoryMonitor.java. We already drop the FaviconCache, and (as far as I can
> tell) we invoke nsMemoryPressure's NS_DispatchMemoryPressure function.

Sounds like you want #2 (or #4) from bug 962967

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

I filed the bugs separately because I felt:
* Tab expiration is "you haven't used this tab in a while, let zombify it"
* Tab discarding is "low memory condition: damn the tabs!"

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: Trigger expiration on heap-minimize v0.1

This patch uses "heap-minimize" to perform expiration, if enabled. It also moves the trigger for expiration-on-newtab into the Tabs class since we have a handler now. I like that for decoupling code.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: More decoupling v0.1

This patch decouples more. The Tab.lastTouchedAt data is a property of the Tab, but Tabs was manipulating it. Since we always update it when we change the Tab.setActive, I just moved the update into setActive.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: Expire sooner v0.1

Based on the long expiration times on Telemetry and the hunch that 3600 secs seems long for mobile usage patterns, this patch reduces the expiration time from 3600 secs (1 hr) to 900 secs (15 mins).

We can watch Telemetry for movement and listen for feedback about zombie tabs being a problem. Keeping in mind that we won't even try to expire tabs unless memory is an issue.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

If we want to be more aggressive, we could make a timer in Tabs with "browser.tabs.expireTime" as the interval and use it as a trigger for expiring tabs.

Comment 8

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

Comment on attachment 8364411 [details] [diff] [review]
Trigger expiration on heap-minimize v0.1

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

::: mobile/android/chrome/content/browser.js
@@ +8285,5 @@
>            this._enableTabExpiration = true;
>            Services.obs.removeObserver(this, "memory-pressure");
> +        } else {
> +          // Use "heap-minimize" as a trigger to expire stale tabs
> +          this.expireLruTab();

Note that expireLruTab expires at most one tab at a time. So the comment here should say "expire the most stale tab, if there is one". Or you could change the behaviour of expireLruTab if you want.

Comment 9

[:Margaret Leibovic]

Comment on attachment 8364411 [details] [diff] [review]
Trigger expiration on heap-minimize v0.1

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

My one concern with this is being so aggressive about expiring tabs that we degrade the tabbed browsing experience for users. However, I see in the expireLruTab implementation that we only expire a tab if it hasn't been touched for more that 3600 seconds, so this means that a user opening a few tabs to research something doesn't need to worry about them expiring while flipping back and forth in one session. So I'm okay with this.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to :Margaret Leibovic from comment #9)
> Comment on attachment 8364411 [details] [diff] [review]
> Trigger expiration on heap-minimize v0.1
> 
> Review of attachment 8364411 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> My one concern with this is being so aggressive about expiring tabs that we
> degrade the tabbed browsing experience for users. However, I see in the
> expireLruTab implementation that we only expire a tab if it hasn't been
> touched for more that 3600 seconds, so this means that a user opening a few
> tabs to research something doesn't need to worry about them expiring while
> flipping back and forth in one session. So I'm okay with this.

The good news is: If you don't have a low-memory device and you don't hit an Android "low-memory" event, you'll never have a tab expire. This only kicks in for known low-memory situations.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Pushed with comment updated
remote:   https://hg.mozilla.org/integration/fx-team/rev/ccadc22d570f
remote:   https://hg.mozilla.org/integration/fx-team/rev/4aed895b4946
remote:   https://hg.mozilla.org/integration/fx-team/rev/2d7a30f65955

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ccadc22d570f
https://hg.mozilla.org/mozilla-central/rev/4aed895b4946
https://hg.mozilla.org/mozilla-central/rev/2d7a30f65955