Last Comment Bug 1331083 - Ship the Large-Allocation header
: Ship the Large-Allocation header
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: Unspecified Unspecified
-- normal (vote)
: mozilla54
Assigned To: Michael Layzell [:mystor]
:
: Andrew Overholt [:overholt]
Mentors:
: 1277066 (view as bug list)
Depends on: 1304140 1329331 1331087 1331525 1332343 1333936 1334309 1334586 1338241
Blocks:
  Show dependency treegraph
 
Reported: 2017-01-13 13:17 PST by Michael Layzell [:mystor]
Modified: 2017-02-27 04:15 PST (History)
10 users (show)
overholt: needinfo? (andrei.vaida)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Enable the Large-Allocation header by default (1008 bytes, patch)
2017-01-18 10:46 PST, Michael Layzell [:mystor]
bugs: review+
lhenry: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
large-alloc-server.py (352 bytes, text/x-python)
2017-02-13 12:35 PST, Michael Layzell [:mystor]
no flags Details

Description User image Michael Layzell [:mystor] 2017-01-13 13:17:14 PST
* The header has been implemented in bug 1304140
* The feature for leaving the process ASAP to avoid loading webpages which don't opt into Large-Allocation in the Large-Allocation process is being done in bug 1329331, and we'll want to land that before shipping.
* According to my conversations with :jukka and his comments like bug 1314098 comment 1, this header's current implementation is good enough to reliably improve allocation successes on 32-bit systems.

I've talked with :mrbkap about the risks of shipping this feature (which effectively ships a limited opt-in form of e10s-multi). He believes that us shipping it is fairly low risk, but there are 2 correctness bugs which he is aware of. These won't be fixed fast enough for us to ship this feature in a reasonable time frame, so we will probably want to make sure to message them as temporary limitations quite clearly when talking about the Large-Allocation header.
* We currently spin up a service worker per process, per origin - rather than one per origin. This means that Large-Allocation loads will have a separate service worker. (bug 1231208)
* We don't notify other processes when localStorage for a given origin changes (bug 1285898)

So long as we message those limitations, I imagine that we can turn this on when bug 1314098 lands, which I am hoping to do very soon.
Comment 1 User image Michael Layzell [:mystor] 2017-01-13 13:21:00 PST
The only other change which we _might_ want to make before/soon after shipping is to ignore the header on 64-bit firefox builds, as we shouldn't have any address space issues, and that saves us spinning up a new process. I think it might be good to not do this, at least for now, however, as that would mean we that developers testing on their 64-bit machines wouldn't notice/be able to reproduce any of the bugs caused by the Large-Allocation header.
Comment 2 User image :Ms2ger (⌚ UTC+1/+2) 2017-01-16 05:39:06 PST
Uh, what is this thing, why are we shipping it, what do other browsers think, where are the intent-to-implement and intent-to-ship?
Comment 3 User image Michael Layzell [:mystor] 2017-01-16 10:06:13 PST
(In reply to :Ms2ger (⌚ UTC+1/+2) from comment #2)
> Uh, what is this thing, why are we shipping it, what do other browsers
> think, where are the intent-to-implement and intent-to-ship?

We have been very uncertain as to whether we wanted to ship this feature until recently. I'll probably send out a intent to implement and ship soon, and it will definitely happen before this feature becomes enabled by default.

The purpose of this header is to counteract the 32-bit out of memory problems which game developers allocating large WASM heaps were running into due to memory fragmentation. The idea is that the Large-Allocation header will act as a hint that the document being loaded wishes to perform one of these large contiguous allocations. We will handle this hint by running the document, if possible without web-visible consequences, in a fresh dedicated process. This process will be newly started, and thus not fragmented, greatly improving the chances of the allocation being successful.
Comment 4 User image Michael Layzell [:mystor] 2017-01-18 10:46:13 PST
Created attachment 8828056 [details] [diff] [review]
Enable the Large-Allocation header by default

This cannot land until bug 1331525 has landed. It turns on the pref for the largeAllocationHeader by default.

MozReview-Commit-ID: DwPyo9kcUnE
Comment 5 User image Olli Pettay [:smaug] 2017-01-18 12:13:40 PST
Is session history working well enough with LA?
Comment 6 User image Olli Pettay [:smaug] 2017-01-18 12:15:25 PST
This definitely needs intent to ship. And multi-e10s folks need to say ok to this too.
Comment 7 User image Michael Layzell [:mystor] 2017-01-18 12:15:43 PST
(In reply to Olli Pettay [:smaug] (review request backlog because of a work week) from comment #5)
> Is session history working well enough with LA?

LA doesn't use the GroupedSHistory infrastructure, and instead relies on the same tools which navigating from content to about:blank or to a file:// URI (which live in the chrome and file URI process respectively) use. We have been shipping this code for a while and it seems fairly reliable (except that it kills the bfcache). I think it should be OK to ship more code depending on it.
Comment 8 User image Michael Layzell [:mystor] 2017-01-18 12:16:25 PST
(In reply to Olli Pettay [:smaug] (review request backlog because of a work week) from comment #6)
> This definitely needs intent to ship. And multi-e10s folks need to say ok to
> this too.

I have a draft intent to ship which I plan to send out very shortly. I don't intend to land this for at least a few days after I send it. If you want I can clear the flag and re r? you in a few days.
Comment 9 User image Olli Pettay [:smaug] 2017-01-18 12:18:38 PST
Comment on attachment 8828056 [details] [diff] [review]
Enable the Large-Allocation header by default

r+, but intent-to-ship may cause some useful discussion about the feature.
So please wait for couple of days after sending intent-to-ship before landing.
And ask r? from mrbkap or gabor.
Comment 10 User image Pulsebot 2017-01-31 11:09:00 PST
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb5ca84bd660
Enable the Large-Allocation header by default, r=smaug
Comment 11 User image Carsten Book [:Tomcat] 2017-02-01 04:16:19 PST
https://hg.mozilla.org/mozilla-central/rev/bb5ca84bd660
Comment 12 User image Michael Layzell [:mystor] 2017-02-01 09:44:41 PST
*** Bug 1277066 has been marked as a duplicate of this bug. ***
Comment 13 User image Michael Layzell [:mystor] 2017-02-01 11:18:43 PST
Comment on attachment 8828056 [details] [diff] [review]
Enable the Large-Allocation header by default

Approval Request Comment

This is part 1 of a 6-bug/7-patch uplift request

[Feature/Bug causing the regression]: We would like to ship the Large-Allocation header in 52/53, and these patches make some final touch-ups which will be required before we can ship.
[User impact if declined]: Increased crash rates of wasm games on 32-bit windows
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Most of these fixups just landed in Nightly.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: 
bug 1331083
bug 1332343
bug 1334210
bug 1333936
bug 1334586
bug 1331087
[Is the change risky?]: Not very
[Why is the change risky/not risky?]: This exposes and enables an opt-in low risk web API which is only supported by Firefox to help avoid OOM problems on 32-bit windows.
[String changes made/needed]: A string was added to dom.properties in bug 1331087
Comment 14 User image Liz Henry (:lizzard) (needinfo? me) 2017-02-02 15:56:10 PST
Comment on attachment 8828056 [details] [diff] [review]
Enable the Large-Allocation header by default

Let's uplift all of this, might be a bit risky but we can pref it off easily.
Comment 15 User image Ryan VanderMeulen [:RyanVM] 2017-02-02 15:58:59 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/8483128d72e6
Comment 16 User image Andrew Overholt [:overholt] 2017-02-09 10:00:34 PST
Andrei, can we get some QA help with this? Michael can provide any details you may need.
Comment 17 User image Andrei Vaida, QA [:avaida] – please ni? me 2017-02-13 07:59:26 PST
(In reply to Andrew Overholt [:overholt] from comment #16)
> Andrei, can we get some QA help with this? Michael can provide any details
> you may need.

Certainly. Michael, could you please point out documentation, specifications or any other information that you think might helps us kick off testing faster?

Leaving the ni? in place for now.
Comment 18 User image Michael Layzell [:mystor] 2017-02-13 12:35:50 PST
Created attachment 8836847 [details]
large-alloc-server.py

(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #17)
> (In reply to Andrew Overholt [:overholt] from comment #16)
> > Andrei, can we get some QA help with this? Michael can provide any details
> > you may need.
> 
> Certainly. Michael, could you please point out documentation, specifications
> or any other information that you think might helps us kick off testing
> faster?
> 
> Leaving the ni? in place for now.

Basically, I would like it if you could do some testing with loading 1 or more documents which have the Large-Allocation header set, and then try doing various things with those documents. For example, there was a bug where dragging a Large-Allocation document into a new window would occasionally not work properly which is being fixed in bug 1338241. 

The main goal here is to ensure that documents which have the Large-Allocation header set act the same as other documents would act. Things like opening links with a _blank target and navigating to other webpages ought to work correctly.

I'll attach a helper python script to this bug which can run a server at localhost:8000 which serves the current working directory with the Large-Allocation header passed along with every file.
Comment 19 User image Michael Layzell [:mystor] 2017-02-13 12:58:18 PST
I should also add that this is the kinda-specification of the feature: https://gist.github.com/mystor/5739e222e398efc6c29108be55eb6fe3

Note You need to log in before you can comment on or make changes to this bug.