1454752 - Add `GeckoRuntime.reduceMemoryUsage()`

Status: NEW

(GeckoView :: General, enhancement, P2)

Description

[Jan Henning [:JanH]]

Gecko's memory pressure handling should be hooked up to work with GeckoView as well.
After a little discussion with snorp [1], a possibility would be to re-use the MemoryMonitor [2] to keep track of the current OS memory pressure level and then notify Gecko as appropriately.
Doing that would approximately involve:
- moving the StorageReducer [3] into a separate class, since it's Fennec-specific code
- moving the MemoryMonitor itself into GeckoView and initialising it from an appropriate location.
- Instead of hard-coding additional actions (other than notifying Gecko) to be taken when memory pressure increases/decreases, let the embedding app set a callback if they want, that gets called with onIncrementMemoryPressure(int newLevel)/onDecrementMemoryPressure(int newLevel) or something like that, then move the current Fennec-specific bits from [4] into that callback.

One thing I'm not sure about - on the JS side in Fennec we currently also turn off the bfcache when Gecko's memory-pressure handling is triggered [5] (and in bug 1335148 I intend to turn the bfcache back on once the memory pressure level tracked by the MemoryMonitor decays back to 0). Do we want to include this behaviour generically for GeckoView as well, or should that remain the responsibility of the embedding app?

[1] https://mozilla.logbot.info/mobile/20180417#c14624244-c14624654
[2] https://dxr.mozilla.org/mozilla-central/rev/0ceabd10aac2272e83850e278c7876f32dbae42e/mobile/android/base/java/org/mozilla/gecko/MemoryMonitor.java
[3] https://dxr.mozilla.org/mozilla-central/rev/0ceabd10aac2272e83850e278c7876f32dbae42e/mobile/android/base/java/org/mozilla/gecko/MemoryMonitor.java#249-285
[4] https://dxr.mozilla.org/mozilla-central/rev/0ceabd10aac2272e83850e278c7876f32dbae42e/mobile/android/base/java/org/mozilla/gecko/MemoryMonitor.java#193-196
[5] https://dxr.mozilla.org/mozilla-central/rev/0ceabd10aac2272e83850e278c7876f32dbae42e/mobile/android/chrome/content/MemoryObserver.js#41

Comment 1

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

(In reply to Jan Henning [:JanH] from comment #0)
After reading MemoryMonitor.java and MemoryObserver.js some, I think maybe we don't want to do any of that automatically. I think we might be assuming too much about the application if we do that. Instead, let's try to expose the things that make sense as GeckoRuntimeSettings or perhaps methods on GeckoRuntime (in the case of gc()).

Then we can let the app listen to memory pressure events and take whatever action it feels appropriate.

Comment 2

[Jan Henning [:JanH]]

Just to make sure - not even for the platform-side memory pressure handling that's triggered by the call to "dispatchMemoryPressure()"?

Comment 3

[Chris Peterson [:cpeterson]]

P2 because this is not a Klar+GeckoView blocker. Klar only supports one tab, so memory pressure is less of a problem, but we want to address this soon for Fennec.

Comment 4

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

I'm elevating this to a P1 because Fenix has multiple tabs and we also have memory-constrained devices for FxTV.

Comment 5

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

We know we need the app to participate in the memory pressure handling, because it needs to make the decision to close background Session instances as a first line. Maybe we can simply expose GeckoRuntime.reduceMemoryUsage() instead of monitoring it in GV ourselves? We would then need to do the backoff ourselves (like re-enabling bfcache). We could expose a similar thing for storage which would cause disk caches to be evicted.

Comment 6

[Jan Henning [:JanH]]

Maybe we can simply expose GeckoRuntime.reduceMemoryUsage() instead of monitoring it in GV ourselves?

In that case (especially if GeckoView will still handle the back-off on its own), repeated calls to reduceMemoryUsage() within a short time frame should be converted into ongoing memory pressure for Gecko, i.e. what so far I didn't got around doing in bug 1457062.

like re-enabling bfcache

bfcache memory-pressure handling would need to be ported to GeckoView in the first place, though.

Comment 7

[Chris Peterson [:cpeterson]]

Adding [gvtv:p2] whiteboard tag because Fire TV devices have limited memory and are likely to be affected by memory pressure issues.

Comment 8

[Chris Peterson [:cpeterson]]

Demoting to priority [geckoview:fenix:p2] because Android Q support is not a Fenix MVP release blocker.

Comment 9

[Jan Henning [:JanH]]

This isn't an Android Q issue though, is it?

Comment 10

[Chris Peterson [:cpeterson]]

(In reply to Jan Henning [:JanH] from comment #9)

This isn't an Android Q issue though, is it?

You're correct. The comment was a copy/paste error, but handling memory pressure is still something we can implement after Fenix MVP.

Comment 11

[Chris Peterson [:cpeterson]]

Removing bug priority to send this bug back to GV triage for discussion.

Comment 13

[Sotaro Ikeda [:sotaro]]

:cpeterson, can this bug could have a priority? Its support might reduce memory usage when gecko uses huge memory like Bug 1639280.

Comment 14

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

I don't think we want GeckoView to handle memory pressure events directly. In a browser, the main thing you want to do in response to a memory pressure event is to close background tabs. GeckoView can't (easily) do that by itself, so instead the app should handle that. Fenix is already doing this today.

We might still want to expose a reduceMemory() method on GeckoRuntime, though. This would do the same thing as the "minimize memory usage" in "about:memory". I don't think this would help the situation in Bug 1639280, however.

Comment 15

[Jan Henning [:JanH]]

This would do the same thing as the "minimize memory usage" in "about:memory".

That's not quite the same thing as activating Gecko's memory pressure handling, though, is it? "minimize memory usage" only sends a bunch of "heap-minimize" notifications.

Comment 16

[Gabriele Svelto [:gsvelto]]

(In reply to Jan Henning [:JanH] from comment #15)

That's not quite the same thing as activating Gecko's memory pressure handling, though, is it? "minimize memory usage" only sends a bunch of "heap-minimize" notifications.

That's correct, in Fennec however both things were conflated into one: we sent the memory-pressure events and also used them to discard tabs. I also implemented that using a more generic mechanism in bug 675539 - still tied to memory-pressure events but had to disable it due to unpleasant issues it was causing. It's behind a pref now so by default sending memory-pressure events won't unload tabs unless we instruct Firefox or the GeckoView-based application to do so. Regular memory-pressure events will purge caches, trim database buffers, jettison invisible images and so on.

Comment 17

[Chris Peterson [:cpeterson]]

(In reply to Sotaro Ikeda [:sotaro] from comment #13)

:cpeterson, can this bug could have a priority? Its support might reduce memory usage when gecko uses huge memory like Bug 1639280.

I no longer work on GeckoView.

(In reply to James Willcox (:snorp) ([e-mail]) (he/him) from comment #14)

I don't think we want GeckoView to handle memory pressure events directly. In a browser, the main thing you want to do in response to a memory pressure event is to close background tabs. GeckoView can't (easily) do that by itself, so instead the app should handle that. Fenix is already doing this today.

We might still want to expose a reduceMemory() method on GeckoRuntime, though. This would do the same thing as the "minimize memory usage" in "about:memory". I don't think this would help the situation in Bug 1639280, however.

snorp, if you don't want GeckoView to handle memory pressure events directly, can this bug be WONTFIX'd? Or do you want to morph this bug into the "Expose a reduceMemory() method on GeckoRuntime" feature request?

Comment 18

[Kelsey Gilbert [:jgilbert]]

(In reply to James Willcox (:snorp) ([e-mail]) (he/him) from comment #14)

I don't think we want GeckoView to handle memory pressure events directly. In a browser, the main thing you want to do in response to a memory pressure event is to close background tabs. GeckoView can't (easily) do that by itself, so instead the app should handle that. Fenix is already doing this today.

We might still want to expose a reduceMemory() method on GeckoRuntime, though. This would do the same thing as the "minimize memory usage" in "about:memory". I don't think this would help the situation in Bug 1639280, however.

We need to hook up CC to the OS memory-pressure/killer events. I'm pretty surprised we don't have that hooked up, since that's an MVP criteria for me to avoid users crashing. Do we CC when we close tabs? Phones are memory-poor environments, so this certainly would explain some of the crashes I get, if not.

Comment 19

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

Hmm so yeah we probably do want to morph this into a bug for exposing GeckoRuntime.reduceMemoryUsage(). I've changed it.

Comment 21

[Chris Peterson [:cpeterson]]

Enhancements should have severity N/A.