Open Bug 1454752 Opened 6 years ago Updated 2 years ago

Add `GeckoRuntime.reduceMemoryUsage()`

Categories

(GeckoView :: General, enhancement, P2)

All
Android
enhancement

Tracking

(firefox66 wontfix, firefox67 fix-optional, firefox68 fix-optional)

Tracking Status
firefox66 --- wontfix
firefox67 --- fix-optional
firefox68 --- fix-optional

People

(Reporter: JanH, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [geckoview:m93?])

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
(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.
Just to make sure - not even for the platform-side memory pressure handling that's triggered by the call to "dispatchMemoryPressure()"?
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.
Priority: -- → P2
Product: Firefox for Android → GeckoView

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

Priority: P2 → P1
Whiteboard: [geckoview:fenix:m4]
Assignee: nobody → snorp

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.

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.

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

Whiteboard: [geckoview:fenix:m4] → [geckoview:fenix:m4][gvtv:p2]

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

Whiteboard: [geckoview:fenix:m4][gvtv:p2] → [geckoview:fenix:p2][gvtv:p2]

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

(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.

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

Priority: P1 → --
Priority: -- → P2
Whiteboard: [geckoview:fenix:p2][gvtv:p2] → [geckoview:fenix:p2] [gvtv:p2]
Rank: 33
Whiteboard: [geckoview:fenix:p2] [gvtv:p2] → [gvtv:p2]
Blocks: 1639280

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

Flags: needinfo?(cpeterson)

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.

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.

(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.

(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) (jwillcox@mozilla.com) (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?

Flags: needinfo?(cpeterson) → needinfo?(snorp)

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (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.

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

Flags: needinfo?(snorp)
Summary: GeckoView memory pressure handling → Add `GeckoRuntime.reduceMemoryUsage()`
Priority: P2 → P3
Assignee: snorp → nobody
Whiteboard: [gvtv:p2] → [geckoview:m93?]
Priority: P3 → P2
Severity: normal → S3
Rank: 33 → 222

Enhancements should have severity N/A.

Severity: S3 → N/A
You need to log in before you can comment on or make changes to this bug.