1701368 - Tab unloading should precede other memory pressure observers

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P1)

Description

[Haik Aftandilian [:haik]]

Tab unloading (not enabled at this time) is implemented as a memory-pressure observer. Given that tab unloading may reduce memory use, we should experiment with unloading tabs first and then notifying memory-pressure observers if we are still in a low memory state.

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

Note that the first time we detect a low-memory situation we send a memory-pressure event with a low-memory payload. If the low memory situation persists further events have the low-memory-ongoing payload instead.

Currently all listeners tend to respond to the latter but we might switch this around: we make tab unloader respond to low-memory and everything else to low-memory-ongoing. This however does not guarantee that the low-memory-ongoing listeners will ever run.

An alternative would be to augment our nsIObserver implementation so that it's possible to prepend listeners (we currently only support appending them). Since listeners are called in the order they appear in the list prepending the tab unloader to all other listeners would guarantee it is always called first. Arguable this second solution would be simpler but maybe slightly more brittle.

Comment 2

[Chris Peterson [:cpeterson]]

Tracking for Fission M8 milestone because Toshi says this bug is a blocker for enabling tab unloading in bug 1587762 (which we are also tracking for Fission M8).

Comment 3

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1701368 - Part1: Extract nsAvailableMemoryWatcher as AvailableMemoryWatcherWin.cpp. r=gsvelto

This patch splits nsAvailableMemoryWatcher into 1) an nsISupports-derived class
nsAvailableMemoryWatcherBase and 2) a platform-specific class nsAvailableMemoryWatcher,
taking out the 2) part as a new file AvailableMemoryWatcherWin.cpp without any change.

Test cases for nsAvailableMemoryWatcher will be added by a subsequent patch.

Comment 4

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1701368 - Part2: Clean up nsAvailableMemoryWatcher. r=gsvelto

1. Use nsAutoHandle instead of a raw HANDLE
2. Add a dtor with MOZ_ASSERT
3. Prevent double init
4. Cache nsAvailableMemoryWatcher::mObserverSvc

Depends on D117669

Comment 5

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1701368 - Part3: Add NS_DispatchEventualMemoryPressure equivalent to nsIMemory. r=gsvelto

This enables JS code to post a memory-pressure event.

Depends on D117670

Comment 6

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1701368 - Part4: Make the nsAvailableMemoryWatcher timer commit-space driven. r=gsvelto

With this patch, the first OnLowMemory is triggered by the memory resource
notification which is based on the available physical memory, but subsequent
OnLowMemory or OnHighMemory is triggered by the timer based on the
available commit space.

The key part of this change is the if block in nsAvailableMemoryWatcher::Notify,
where we use a single condition IsCommitSpaceLow() to declare either Low or High.
With this, OnLowMemory is called not only in the main thread but also in a worker
thread, StartPollingIfUserInteracting also needs a lock to protect mPolling.
We don't need QueryMemoryResourceNotification anymore.

Depends on D117671

Comment 7

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1701368 - Part6: Tab unloading precedes memory pressure events. r=gsvelto

This is the main part to address bug 1701368.

Before this patch, nsAvailableMemoryWatcher directly broadcasted
a memory-pressure event when we enter into a low-memory situation and
TabUnloader unloaded a tab in response to the memory-pressure message.

This patch introduces a new message "memory-watcher-notification" for
nsAvailableMemoryWatcher to send to TabUnloader without involving
other memory-pressure listeners.

With this, nsAvailableMemoryWatcher sends "memory-watcher-notification"
with a subtopic "OnLowMemory" or "OnHighMemory" accordingly. TabUnloader
is responsible for an actual action: unload a tab or send a memory-pressure.

To achieve that, this patch implements a state machine in TabUnloader,
consisting of three states: Normal, LowMemory, and MemoryPressured.
State transition happens in response to "memory-watcher-notification" from
nsAvailableMemoryWatcher. Currently we define seven transitions:

1. Normal --(OnLowMemory)--> LowMemory
2. Normal --(OnLowMemory)--> MemoryPressured
3. LowMemory --(OnLowMemory)--> LowMemory
4. LowMemory --(OnLowMemory)--> MemoryPressured
5. LowMemory --(OnHighMemory)--> Normal
6. MemoryPressured --(OnLowMemory)--> MemoryPressured
7. MemoryPressured --(OnHighMemory)--> Normal

Depends on D117672

Comment 8

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1701368 - Part7: Add GTest to test nsAvailableMemoryWatcher. r=gsvelto

Depends on D117673

Comment 9

[Chris Peterson [:cpeterson]]

Tab unloading is a nice-to-have for our Fission Release experiment, but doesn't need to block it. I will move this bug from Fission Milestone M8 to MVP.

Comment 10

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1701368 - Part5: Convert nsAvailableMemoryWatcher to an XPCOM object. r=gsvelto

This patch introduces an XPCOM object which is represented by the single instance of
nsAvailableMemoryWatcherBase so that nsAvailableMemoryWatcher can synchronously
access TabUnloader.

We currently implement a watcher class for Windows only. For other platforms, what
we need to do is to define a class inherinting nsAvailableMemoryWatcherBase and
a simple factory method CreateAvailableMemoryWatcher() returning an instance of
that class.

Depends on D117672

Comment 11

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1701368 - Part3: Remove the MemPressure_Ongoing request. r=gsvelto

We had NS_DispatchMemoryPressure and NS_DispatchEventualMemoryPressure
to dispatch a memory-pressure event which took MemPressure_New and
MemPressure_Ongoing to translate into "low-memory" and "low-memory-ongoing"
message respectively.

With that model, we could end up sending a wrong message if somebody
called the API with MemPressure_Ongoing without sending MemPressure_New.
To avoid that, this patch removes MemPressure_Ongoing and makes
the API decide whether it should dispatch a "new" event or "ongoing" event.

Depends on D117670

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/feca7daa9c96
Part1: Extract nsAvailableMemoryWatcher as AvailableMemoryWatcherWin.cpp. r=gsvelto
https://hg.mozilla.org/integration/autoland/rev/d773413513cf
Part2: Clean up nsAvailableMemoryWatcher.  r=gsvelto
https://hg.mozilla.org/integration/autoland/rev/98046adfc310
Part3: Remove the MemPressure_Ongoing request.  r=gsvelto
https://hg.mozilla.org/integration/autoland/rev/f3a475931a13
Part4: Make the nsAvailableMemoryWatcher timer commit-space driven.  r=gsvelto
https://hg.mozilla.org/integration/autoland/rev/38fb1e114a53
Part5: Convert nsAvailableMemoryWatcher to an XPCOM object.  r=gsvelto
https://hg.mozilla.org/integration/autoland/rev/907317341862
Part6: Tab unloading precedes memory pressure events.  r=gsvelto
https://hg.mozilla.org/integration/autoland/rev/3be9f410b666
Part7: Add GTest to test nsAvailableMemoryWatcher.  r=gsvelto

Comment 13

[Atila Butkovits]

Backed out for causing build bustages on TestMemoryPressure.cpp.

Backout link: https://hg.mozilla.org/integration/autoland/rev/75dcd0588558a4e3723db93151d8f15c5d6a215b

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&searchStr=linux%2Cx64%2Cdebug%2Cbuild-linux64-base-toolchains%2Fdebug%2Cbb&revision=3be9f410b666baf8583b95cc2911d45ee0b5292e&selectedTaskRun=RdP0Sto8Q8m8SpVwE2LP9g.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=344504482&repo=autoland&lineNumber=68241

Comment 14

[Toshihito Kikuchi [:toshi]]

Updated the part3 to fix the compile warning in TestMemoryPressure.cpp and updated the part6 to fix the error in browser_TabUnloader.js during test-windows10-64-qr/debug-test-verify-fis-e10s TV-fis.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1251a4fedbd0
Part1: Extract nsAvailableMemoryWatcher as AvailableMemoryWatcherWin.cpp. r=gsvelto
https://hg.mozilla.org/integration/autoland/rev/857bb33999bd
Part2: Clean up nsAvailableMemoryWatcher.  r=gsvelto
https://hg.mozilla.org/integration/autoland/rev/278e8d0bd4dc
Part3: Remove the MemPressure_Ongoing request.  r=gsvelto
https://hg.mozilla.org/integration/autoland/rev/a2c139554e9e
Part4: Make the nsAvailableMemoryWatcher timer commit-space driven.  r=gsvelto
https://hg.mozilla.org/integration/autoland/rev/9c9913e934af
Part5: Convert nsAvailableMemoryWatcher to an XPCOM object.  r=gsvelto
https://hg.mozilla.org/integration/autoland/rev/b88c3bce751e
Part6: Tab unloading precedes memory pressure events.  r=gsvelto
https://hg.mozilla.org/integration/autoland/rev/f987c607501f
Part7: Add GTest to test nsAvailableMemoryWatcher.  r=gsvelto

Comment 16

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/1251a4fedbd0
https://hg.mozilla.org/mozilla-central/rev/857bb33999bd
https://hg.mozilla.org/mozilla-central/rev/278e8d0bd4dc
https://hg.mozilla.org/mozilla-central/rev/a2c139554e9e
https://hg.mozilla.org/mozilla-central/rev/9c9913e934af
https://hg.mozilla.org/mozilla-central/rev/b88c3bce751e
https://hg.mozilla.org/mozilla-central/rev/f987c607501f