Nika Layzell [:nika] (ni? for response) Assignee
	Description • 7 years ago

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

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 1 • 7 years ago

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.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 2 • 7 years ago

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?

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 3 • 7 years ago

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

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 4 • 7 years ago

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

MozReview-Commit-ID: DwPyo9kcUnE

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 7 years ago

Is session history working well enough with LA?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 7 years ago

This definitely needs intent to ship. And multi-e10s folks need to say ok to this too.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 7 • 7 years ago

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

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 8 • 7 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 9 • 7 years ago

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.

	Pulsebot
	Comment 10 • 7 years ago

Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb5ca84bd660
Enable the Large-Allocation header by default, r=smaug

	Carsten Book [:Tomcat]
	Comment 11 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/bb5ca84bd660

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 13 • 7 years ago

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

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 14 • 7 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/8483128d72e6

	Andrew Overholt [:overholt]
	Comment 16 • 7 years ago

Andrei, can we get some QA help with this? Michael can provide any details you may need.

	Andrei Vaida [:avaida]
	Comment 17 • 7 years ago

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

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 18 • 7 years ago

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

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 19 • 7 years ago

I should also add that this is the kinda-specification of the feature: https://gist.github.com/mystor/5739e222e398efc6c29108be55eb6fe3

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 20 • 7 years ago

I haven't heard any feedback (positive or negative) about this so far but did discuss it with overholt. 
We can ship it as the default to beta and do some testing after 53 goes to beta. 

mystor, can you suggest a release note for beta 53?

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 21 • 7 years ago

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #20)
> I haven't heard any feedback (positive or negative) about this so far but
> did discuss it with overholt. 
> We can ship it as the default to beta and do some testing after 53 goes to
> beta. 
> 
> mystor, can you suggest a release note for beta 53?

I'm not great at writing release notes, but perhaps:

Added 32-bit Windows support for Large-Allocation header workaround for memory fragmentation issues with large WASM games.

Potentially we could add a reference to the MDN article: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Large-Allocation

	Andrei Vaida [:avaida]
	Comment 22 • 7 years ago
testplan

[Test Plan]:
https://wiki.mozilla.org/QA/Large-Allocation_header

	Chris Mills [:cmills]
	Comment 23 • 7 years ago

I've updated the support information on the Large-Allocation header reference page:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Large-Allocation (changes not live yet; see https://github.com/mdn/browser-compat-data/pull/152)

I've also added a note to the Fx54 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/54

And a link to the WebAssembly landing page:

https://developer.mozilla.org/en-US/docs/WebAssembly

Let me know if this all looks OK, or if you think anything else is needed. Thanks!

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 24 • 7 years ago

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #23)
> I've updated the support information on the Large-Allocation header
> reference page:
> 
> https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Large-Allocation
> (changes not live yet; see
> https://github.com/mdn/browser-compat-data/pull/152)
> 
> I've also added a note to the Fx54 release notes:
> 
> https://developer.mozilla.org/en-US/Firefox/Releases/54

I believe that this feature is shipping in 53, not 54.

> And a link to the WebAssembly landing page:
> 
> https://developer.mozilla.org/en-US/docs/WebAssembly
> 
> Let me know if this all looks OK, or if you think anything else is needed.
> Thanks!

	Chris Mills [:cmills]
	Comment 25 • 7 years ago

(In reply to Michael Layzell [:mystor] from comment #24)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #23)
> > 
> > https://developer.mozilla.org/en-US/Firefox/Releases/54
> 
> I believe that this feature is shipping in 53, not 54.

Ah, thanks for the clarification. I've updated the support info, and moved the note to the Fx 53 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/53#HTTPNetworking