1376137 - MemoryMonitor has Android memory pressure levels the wrong way around

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Jan Henning [:JanH]]

According to the Android documentation (https://developer.android.com/reference/android/content/ComponentCallbacks2.html), the initial warning when we're in foreground is TRIM_MEMORY_RUNNING_MODERATE, which is then followed by TRIM_MEMORY_RUNNING_LOW and finally TRIM_MEMORY_RUNNING_CRITICAL.

Our current logic (https://dxr.mozilla.org/mozilla-central/rev/92dc60b522d81862e52bff5cdb1b698eb5608658/mobile/android/base/java/org/mozilla/gecko/MemoryMonitor.java#120) however has the relationship to our own pressure levels swapped around - TRIM_MEMORY_RUNNING_MODERATE triggers MEMORY_PRESSURE_MEDIUM, while TRIM_MEMORY_RUNNING_LOW only triggers MEMORY_PRESSURE_LOW.

This might also explain why some users have complained that we unload tabs too early, because Gecko-side memory pressure is dispatched for MEMORY_PRESSURE_MEDIUM and higher.

Comment 1

[Jan Henning [:JanH]]

Attachment: Bug 1376137 - Fix Android memory pressure mapping.

The lowest foreground memory pressure level we receive is TRIM_MEMORY_RUNNING_MODERATE, which is then followed by TRIM_MEMORY_RUNNING_LOW, so it's the latter that should trigger MEMORY_PRESSURE_MEDIUM.

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

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8881965 [details]
Bug 1376137 - Fix Android memory pressure mapping.

https://reviewboard.mozilla.org/r/153032/#review158232

I vaguely remember some weird reason it was done this way, but I'm not finding it immediately, so I think this is change is fine.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0afe480c1642
Fix Android memory pressure mapping. r=snorp

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0afe480c1642

Comment 5

[Richard Newman [:rnewman]]

I think Bug 1106827 might be the context you're looking for, snorp.

Comment 6

[Jan Henning [:JanH]]

Comment on attachment 8881965 [details]
Bug 1376137 - Fix Android memory pressure mapping.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1106827
[User impact if declined]: We start unloading tabs at the slightest hint of memory pressure from the OS instead of waiting for the following OS memory pressure level (where it then actually makes sense to start reducing our memory usage).
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Just changed the mapping between OS and internal memory pressure levels to have the correct ordering according to the Android documentation.
[String changes made/needed]: none

Comment 7

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8881965 [details]
Bug 1376137 - Fix Android memory pressure mapping.

fennec fix for memory pressure handling, beta55+

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ee49e510bc53