Closed Bug 1545593 Opened 7 months ago Closed 7 months ago

Enable tab-unloading on low-memory on release

Categories

(Firefox :: Tabbed Browser, task, P1)

task

Tracking

()

RESOLVED FIXED
Firefox 68
Tracking Status
firefox67 --- fixed
firefox68 --- fixed

People

(Reporter: gsvelto, Assigned: gsvelto)

References

Details

Attachments

(1 file)

Tab-unloading-on-low-memory (bug 675539) has been in beta for a while without apparently causing regressions so it's time to enable it unconditionally.

OOM crashes seems ~10% lower in absolute numbers than in the 66.0 beta cycle but I couldn't gauge the crash rate per usage hours metric yet. Still the impact is visible even though it's less than I had hoped.

Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Priority: -- → P1

Hey gsvelto, the code looks fine, but just out of curiosity, has anyone from Product signed off on letting this ride out to release?

Flags: needinfo?(gsvelto)

(In reply to Mike Conley (:mconley) (:⚙️) from comment #2)

Hey gsvelto, the code looks fine, but just out of curiosity, has anyone from Product signed off on letting this ride out to release?

Actually no... Should I do that? The reason why we originally made it nightly-only was that this code wasn't exercised outside of extensions and we feared we might hit some unexpected issues so we wanted a quick way to turn it off. Since we haven't hit any issue I just thought it was safe to enable it everywhere.

Flags: needinfo?(gsvelto)

Yeah, I think we might want to get their OK on this, especially because in the worst case, it means users are going to be flipping back to tabs and having them reload unexpectedly.

I've asked kev to weigh in on this.

Flags: needinfo?(kev)

Hey gsvelto - could you point me at how we determine low memory conditions? I have a couple quick questions around this, and need a little info that I'm hoping are easy asks:

- what conditions are required to trigger low memory 
- are there any other conditions we check for outside of what's identified in https://searchfox.org/mozilla-central/source/browser/modules/TabUnloader.jsm#49-51 to make a determination to maintain a tab?

My biggest concerns are around user experience, specifically if there's a tab w/ a service worker or other background action/process/script that may need to be re-instantiated or authenticated, and what that would look like to the user on resumption. Suspect it's similar to restarts, but wanted to be sure.

I'd also ask for more details on whether or not we're still looking at only enabling this through an extension api, or if it'd be going beyond that (e.g. by default, everywhere). I'll keep NI open for me for now.

Thanks!

(In reply to Kev Needham [:kev] from comment #5)

Hey gsvelto - could you point me at how we determine low memory conditions? I have a couple quick questions around this, and need a little info that I'm hoping are easy asks:

- what conditions are required to trigger low memory 

A low memory condition is detected if the user is actively interacting with Firefox and available commit space [1] or available virtual memory [2] drops to less than 384MiB. Commit space is the amount of physical memory plus swap space available and is the best measure of "free memory" on Windows; this is used both in 32- and 64-bit builds. The available virtual memory threshold on the other hand is only used on 32-bit Windows. If the user is not interacting with Firefox (e.g. it was left in the background) we don't track available memory and so this machinery will not be triggered even if memory starts running low. The reasoning behind this is that we need to poll available memory and doing it when Firefox is idle could needlessly drain the user's battery.

- are there any other conditions we check for outside of what's identified in https://searchfox.org/mozilla-central/source/browser/modules/TabUnloader.jsm#49-51 to make a determination to maintain a tab?

No, but the discardBrowser() method does not automatically succeed so depending on the tab's state it might not be unloaded even if we try to.

My biggest concerns are around user experience, specifically if there's a tab w/ a service worker or other background action/process/script that may need to be re-instantiated or authenticated, and what that would look like to the user on resumption. Suspect it's similar to restarts, but wanted to be sure.

Behavior is going to be the same as restarting the browser. After being unloaded the tabs will be restored via session restore like during a restart.

I'd also ask for more details on whether or not we're still looking at only enabling this through an extension api, or if it'd be going beyond that (e.g. by default, everywhere). I'll keep NI open for me for now.

Tab unloading functionality is available via an extension API [3] and here we use the same underlying mechanism to implement this.

[1] https://searchfox.org/mozilla-central/rev/ec489aa170b6486891cf3625717d6fa12bcd11c1/xpcom/base/AvailableMemoryTracker.cpp#63
[2] https://searchfox.org/mozilla-central/rev/ec489aa170b6486891cf3625717d6fa12bcd11c1/xpcom/base/AvailableMemoryTracker.cpp#58
[3] https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/tabs/discard

Thanks for the additional info. The utility and value is understood, and to confirm:

  • this change is not extension-specific; while there is a tab unload function, the specific request here is to enable auto-unloading and letting it ride to release
  • has been in beta for some time and is considered release ready

Could you confirm?

My main concern is around a lack of clarity with what kind of feedback we've been getting from users directly, whether there are any common cases we should be aware of that may impact users negatively, and how many users this is likely to potentially affect (e.g. what percentage of profiles do we see that we think will hit the low memory condition). I'd also like to understand if we know the potential impact; you mention that we reduced OOM crashes by 10%, and I was hoping you could point me at what that means in terms of absolute sessions or installs.

I'd like to ensure we have a good idea of potential impact and risk in terms of users likely to be affected, and whether we've been seeing feedback/complaints that may be related. Apologies for asking the additional information; if these are readily available I'm happy to go through it asap. If we're confident the feature is ready, I mostly want to make sure we won't be surprising end-users by subjecting them to higher wait times (which is better than crashing, I agree :) ).

Flags: needinfo?(kev) → needinfo?(gsvelto)

(In reply to Kev Needham [:kev] from comment #7)

Thanks for the additional info. The utility and value is understood, and to confirm:

  • this change is not extension-specific; while there is a tab unload function, the specific request here is to enable auto-unloading and letting it ride to release
  • has been in beta for some time and is considered release ready

Could you confirm?

Yes, I've compared the same beta releases for 66.0 and 67.0 to compare the OOM crash rate and on 67 it's roughty 10% lower. When we enabled it on nightly the effect was also visible as a marked reduction in OOM crash pings:

https://sql.telemetry.mozilla.org/queries/61701#158787

On beta I hoped to be able to calculate the number of OOM crashes per kusage hours which is a better proxy of the actual crash rate. I had an old query that did that but the dataset has been since removed and I wasn't able to update it. I can also confirm that this has been in beta since mid-March and it doesn't seem to have caused any regressions or other user-facing problems.

My main concern is around a lack of clarity with what kind of feedback we've been getting from users directly, whether there are any common cases we should be aware of that may impact users negatively, and how many users this is likely to potentially affect (e.g. what percentage of profiles do we see that we think will hit the low memory condition). I'd also like to understand if we know the potential impact; you mention that we reduced OOM crashes by 10%, and I was hoping you could point me at what that means in terms of absolute sessions or installs.

From a user standpoint this isn't very visible unless their Firefox was crashing all the time because of lack of memory in which case it should be crashing less often. The only thing they'll notice is that going back to a tab might cause it to be reloaded if it was unloaded to "save" Firefox from crashing. Since this is integrated with session restore and the mechanism has already been tested in extensions I don't expect any "surprising" behavior.

I'd like to ensure we have a good idea of potential impact and risk in terms of users likely to be affected, and whether we've been seeing feedback/complaints that may be related. Apologies for asking the additional information; if these are readily available I'm happy to go through it asap. If we're confident the feature is ready, I mostly want to make sure we won't be surprising end-users by subjecting them to higher wait times (which is better than crashing, I agree :) ).

The worst case I can think of is unloading a tab that has some important data that session restore can't handle (e.g. dynamic forms, maybe session data). That data will be lost, but it would be lost anyway if we crashed so it shouldn't be worse. On this topic I'd like to stress that this mechanism kicks in only when we're left with a really small amount of free memory; it will not be a common occurrence. OOM crashes represent only ~10% of all crashes.

Flags: needinfo?(gsvelto)

Hi kev, does comment 8 have enough information to make a call on whether or not we should let this ride out in 68?

Flags: needinfo?(kev)

Given comments, in particular the low memory threshold of 384MB where user pert is likely affected pretty dramatically already, and the worst case being that the user has to reload vs. An OOM crash I’m on board with proceeding. I’d like to make sure support is aware, and we monitor crash rates and feedback post release.

Mconca can provide additional info as needed, I’m ooo unexpectedly today (May 1)

Flags: needinfo?(kev)
Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c2837abe3cd2
Enable tab unloading in low-memory scenarios unconditionally r=mconley

Comment on attachment 9059379 [details]
Bug 1545593 - Enable tab unloading in low-memory scenarios unconditionally

Beta/Release Uplift Approval Request

  • User impact if declined: Users of the 67 release will experience more OOM crashes.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This feature has been tested on both nightly and beta. This particular patch removes the beta-only restriction, but shouldn't change the behavior from what we're already testing in beta.
  • String changes made/needed: n/a
Attachment #9059379 - Flags: approval-mozilla-beta?
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68

(In reply to Gabriele Svelto [:gsvelto] from comment #8)

(In reply to Kev Needham [:kev] from comment #7)

  • has been in beta for some time and is considered release ready

Could you confirm?

Yes, I've compared the same beta releases for 66.0 and 67.0 to compare the OOM crash rate and on 67 it's roughty 10% lower. When we enabled it on nightly the effect was also visible as a marked reduction in OOM crash pings:

Gabriele, this feature was behind the EARLY_BETA_OR_EARLIER flag which means that it was in 67 beta up to beta 9, can you confirm that you compared that range of betas with 66 and not the whole beta cycle?

Flags: needinfo?(gsvelto)

Sorry for not providing better data before. Here's a comparison of the first 9 beta release with the number of crash reports for the four most prominent OOM crash signatures. With the exception of a few small outliers the number of crashes on 67.0b has been well under the corresponding releases on 66.0b. I have only absolute numbers at this point but with some signatures showing anywhere from a 10% to a 50% drop I believe we'll see a much lower overall OOM crash rate once this hits release.

OOM | Small

release 66.0 67.0
b1 620 642
b2 684 485
b3 3478 2979
b4 5290 4884
b5 4666 4467
b6 5806 4936
b7 5004 4439
b8 5763 4805
b9 5260 5352

OOM | large | js::AutoEnterOOMUnsafeRegion::crash | js::AutoEnterOOMUnsafeRegion::crash | js::TenuringTracer::moveToTenuredSlow

release 66.0 67.0
b1 77 42
b2 112 28
b3 716 387
b4 1143 645
b5 1088 572
b6 1219 593
b7 1063 517
b8 1452 644
b9 1122 603

OOM | large | NS_ABORT_OOM | nsTSubstring<T>::SetLength

release 66.0 67.0
b1 4 2
b2 6 2
b3 19 14
b4 38 24
b5 33 57
b6 86 26
b7 145 85
b8 18 23
b9 116 53

OOM | large | js::AutoEnterOOMUnsafeRegion::crash | js::AutoEnterOOMUnsafeRegion::crash | js::TenuringTracer::movePlainObjectToTenured

release 66.0 67.0
b1 14 25
b2 17 19
b3 285 263
b4 438 420
b5 439 340
b6 515 391
b7 480 288
b8 632 434
b9 533 388
Flags: needinfo?(gsvelto)

Comment on attachment 9059379 [details]
Bug 1545593 - Enable tab unloading in low-memory scenarios unconditionally

OOM results look good on beta, uplift approved for 67 beta 16, thanks!

Attachment #9059379 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.