Closed Bug 1335148 Opened 7 years ago Closed 6 years ago

Investigate tweaking bfache preferences on Android

Tracking

(firefox62 fixed)

Status:

RESOLVED FIXED

Milestone:

Firefox 62

Tracking Flags:

Tracking

Status

firefox62

---

fixed

People

(Reporter: JanH, Assigned: JanH)

References

Details

Attachments

(4 files)

Bug 1335148 - Part 1: Dynamically determine content viewer count on Android, too. 6 years ago Jan Henning [:JanH] 59 bytes, text/x-review-board-request	bzbarsky : review+ snorp : review+	Details
Bug 1335148 - Part 2: Introduce notification for end of memory pressure. 6 years ago Jan Henning [:JanH] 59 bytes, text/x-review-board-request	gsvelto : review+	Details
Bug 1335148 - Part 3: Notify Gecko and turn bfcache back on when Fennec memory pressure decays to 0. 6 years ago Jan Henning [:JanH] 59 bytes, text/x-review-board-request	snorp : review+	Details
Bug 1335148 - Part 4: Remove unused max_decoded_image_kb pref. 6 years ago Jan Henning [:JanH] 59 bytes, text/x-review-board-request	snorp : review+	Details

Jan Henning [:JanH]

Assignee

Description

•

7 years ago

Since we decided to turn on the bfcache in bug 472368, browser.sessionhistory.max_entries has always remained at 1, meaning we store at most one entry into the bfache.

We should investigate whether we could finally safely increase that value, although I can't say either whether the automatic algorithm (enabled by setting that pref to -1, see https://hg.mozilla.org/mozilla-central/annotate/71224049c0b52ab190564d3ea0eab089a159a4cf/docshell/shistory/nsSHistory.cpp#l263) might or might not still be too optimistic for us, although I guess we could always decide to tweak it with an #ifdef ANDROID.

Our current browser.sessionhistory.contentViewerTimeout will also mean that bfcache entries will expire sooner than on desktop and on a memory pressure event we drop the bfcache completely anyway, which should protect us a bit against excessive memory usage, but we should probably still test first how much of an impact increasing that pref could have.

Speaking of memory pressure, we then disable the bfache for the remainder of the session until the app is restarted (https://dxr.mozilla.org/mozilla-central/rev/71224049c0b52ab190564d3ea0eab089a159a4cf/mobile/android/chrome/content/MemoryObserver.js#41) - maybe we should cautiously try re-enabling it when our internal memory pressure state decays back to 0 (we currently don't have a notification for that, though).

Jan Henning [:JanH]

Assignee

Comment 2

•

6 years ago

Actually I've just realised that the value of 1 was set when Fennec was still running on the N810, which had a whopping 128 MB of memory.
Even on a hand-wavy scale with conservative assumptions that's long enough ago that we should be able to afford a somewhat larger value here.
If initially we still want to be absolutely sure not to overdo things, subtracting 15.9 instead of 14 at [1] will give
 256 MB -> 1
 512 MB -> 2
 768 MB -> 2
1024 MB -> 3
2048 MB -> 5
3072 MB -> 7
4096 MB -> 8

(In reply to Jan Henning [:JanH] from comment #0)
> Our current browser.sessionhistory.contentViewerTimeout will also mean that
> bfcache entries will expire sooner than on desktop
Currently after six minutes, to be precise.


[1] https://dxr.mozilla.org/mozilla-central/rev/de32269720d056972b85f4eec5f0a8286de6e3af/docshell/shistory/nsSHistory.cpp#299

Assignee: nobody → jh+bugzilla

Comment hidden (mozreview-request)

The current limit of at most one bfcache entry on Android dates back to when
Fennec was built for the Nokia N810, which had a whopping 128 MB of memory.
Since a few years have passed since then and mobile device technology has
evolved considerably, it should be safe now to allow a little more than that.

Since web sites sizes might have grown somewhat as well compared to the figure
of 4MB mentioned in CalcMaxTotalViweres(), though and to be absolutely on the
safe side, we still tweak the formula when building for Android, though.

If in the worst case even those assumptions turn out too generous, we will still
be protected by the fact that
- we temporarily disable the bfcache when the OS signals memory pressure, and
- our contentViewerTimeout is set to a lower value than on Desktop, so bfcache
  entries will expire sooner

Review commit: https://reviewboard.mozilla.org/r/241862/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241862/

Comment hidden (mozreview-request)

For Fennec on Android, if we haven't received memory pressure notifications from
the OS for a certain amount of time (in the order of ~15 mins), we assume that
we're no longer under memory pressure. In order to turn the bfcache back on when
that happens, we now want to be able to forward this fact to Gecko as well.

Unfortunately, the way memory pressure is tracked using an atomic variable
doesn't easily allow to fully extend the existing priority rules between "new"
and "ongoing" to include a new "stopping of memory pressure" event. Since we're
not using Dispatch*Eventual*MemoryPressure on Android and therefore the queuing
priority behaviour isn't actually relevant for us, we just ignore that and only
enforce that a pending "new" memory pressure event takes priority over a "stop"
event.

Review commit: https://reviewboard.mozilla.org/r/241864/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241864/

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/241872/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241872/

Comment hidden (mozreview-request)

Since bug 1104622, that pref has in fact been unused, so us setting that value
to 0 on memory pressure didn't really achieve anything.

Instead, the SurfaceCache already does its own memory management and discards
image surfaces when receiving a memory-pressure notification.

Review commit: https://reviewboard.mozilla.org/r/241866/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241866/

James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)

Comment 7

•

6 years ago

mozreview-review

Comment on attachment 8973511 [details]
Bug 1335148 - Part 1: Dynamically determine content viewer count on Android, too.

https://reviewboard.mozilla.org/r/241862/#review247960

Attachment #8973511 - Flags: review?(snorp) → review+

James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)

Comment 8

•

6 years ago

mozreview-review

Comment on attachment 8973512 [details]
Bug 1335148 - Part 2: Introduce notification for end of memory pressure.

https://reviewboard.mozilla.org/r/241864/#review247962

Attachment #8973512 - Flags: review+

James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)

Comment 9

•

6 years ago

mozreview-review

Comment on attachment 8973513 [details]
Bug 1335148 - Part 3: Notify Gecko and turn bfcache back on when Fennec memory pressure decays to 0.

https://reviewboard.mozilla.org/r/241872/#review247964

Attachment #8973513 - Flags: review?(snorp) → review+

James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)

Comment 10

•

6 years ago

mozreview-review

Comment on attachment 8973514 [details]
Bug 1335148 - Part 4: Remove unused max_decoded_image_kb pref.

https://reviewboard.mozilla.org/r/241866/#review247966

Attachment #8973514 - Flags: review?(snorp) → review+

Nicholas Nethercote [inactive]

Updated

•

6 years ago

Attachment #8973512 - Flags: review?(n.nethercote) → review?(gsvelto)

Gabriele Svelto [:gsvelto] (PTO)

Comment 11

•

6 years ago

mozreview-review

Comment on attachment 8973512 [details]
Bug 1335148 - Part 2: Introduce notification for end of memory pressure.

https://reviewboard.mozilla.org/r/241864/#review248176

This is fine for now. I'm introducing a new way to monitor memory usage on desktop platforms in bug 1451005 which will make low-memory detection more robust and will allow me to gather more data on low-memory scenarios. So if we need to change the queuing behavior it's better to do it afterwards, when we'll have more usage data for these notifications.

Attachment #8973512 - Flags: review?(gsvelto) → review+

Comment hidden (mozreview-request)

Comment on attachment 8973511 [details]
Bug 1335148 - Part 1: Dynamically determine content viewer count on Android, too.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241862/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8973512 [details]
Bug 1335148 - Part 2: Introduce notification for end of memory pressure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241864/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8973513 [details]
Bug 1335148 - Part 3: Notify Gecko and turn bfcache back on when Fennec memory pressure decays to 0.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241872/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8973514 [details]
Bug 1335148 - Part 4: Remove unused max_decoded_image_kb pref.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241866/diff/1-2/

Boris Zbarsky [:bzbarsky]

Comment 16

•

6 years ago

mozreview-review

Comment on attachment 8973511 [details]
Bug 1335148 - Part 1: Dynamically determine content viewer count on Android, too.

https://reviewboard.mozilla.org/r/241862/#review248392

r=me

::: docshell/shistory/nsSHistory.cpp:46
(Diff revision 2)
>  #define CONTENT_VIEWER_TIMEOUT_SECONDS "browser.sessionhistory.contentViewerTimeout"
>  
>  // Default this to time out unused content viewers after 30 minutes
>  #define CONTENT_VIEWER_TIMEOUT_SECONDS_DEFAULT (30 * 60)
>  
> +#ifdef ANDROID

This really needs to be somewhere near the comment that maps RAM amounts to numbers of viewers.  Otherwise it's really not clear where these magic numbers are coming from.

Attachment #8973511 - Flags: review?(bzbarsky) → review+

Comment hidden (mozreview-request)

Comment on attachment 8973511 [details]
Bug 1335148 - Part 1: Dynamically determine content viewer count on Android, too.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241862/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8973512 [details]
Bug 1335148 - Part 2: Introduce notification for end of memory pressure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241864/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8973513 [details]
Bug 1335148 - Part 3: Notify Gecko and turn bfcache back on when Fennec memory pressure decays to 0.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241872/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8973514 [details]
Bug 1335148 - Part 4: Remove unused max_decoded_image_kb pref.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241866/diff/2-3/

Pulsebot

Comment 21

•

6 years ago

Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/d8e7c56a2a85
Part 1: Dynamically determine content viewer count on Android, too. r=bz,snorp
https://hg.mozilla.org/integration/autoland/rev/6e79931930ee
Part 2: Introduce notification for end of memory pressure. r=gsvelto,snorp
https://hg.mozilla.org/integration/autoland/rev/36e95b6ba7a6
Part 3: Notify Gecko and turn bfcache back on when Fennec memory pressure decays to 0. r=snorp
https://hg.mozilla.org/integration/autoland/rev/e5332f31998b
Part 4: Remove unused max_decoded_image_kb pref. r=snorp

Natalia Csoregi [:nataliaCs]

Comment 22

•

6 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/d8e7c56a2a85
https://hg.mozilla.org/mozilla-central/rev/6e79931930ee
https://hg.mozilla.org/mozilla-central/rev/36e95b6ba7a6
https://hg.mozilla.org/mozilla-central/rev/e5332f31998b

Status: NEW → RESOLVED

Closed: 6 years ago

status-firefox62: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → Firefox 62

Nobody; OK to take it and work on it

Updated

•

3 years ago

Product: Firefox for Android → Firefox for Android Graveyard

You need to log in before you can comment on or make changes to this bug.