Closed Bug 1331083 Opened 7 years ago Closed 7 years ago

Ship the Large-Allocation header

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: nika, Assigned: nika)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

Enable the Large-Allocation header by default
1008 bytes, patch
smaug
: review+
lizzard
: approval-mozilla-aurora+
Details | Diff | Splinter Review
large-alloc-server.py
352 bytes, text/x-python
Details 
* 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.
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.
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?
(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.
Depends on: 1331525
This cannot land until bug 1331525 has landed. It turns on the pref for the largeAllocationHeader by default.

MozReview-Commit-ID: DwPyo9kcUnE
Attachment #8828056 - Flags: review?(bugs)
Is session history working well enough with LA?
This definitely needs intent to ship. And multi-e10s folks need to say ok to this too.
(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.
(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 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.
Attachment #8828056 - Flags: review?(bugs) → review+
Depends on: 1332343
No longer blocks: 1331087
Depends on: 1331087
Depends on: 1333936
Depends on: 1334309
Depends on: 1334586
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb5ca84bd660
Enable the Large-Allocation header by default, r=smaug
https://hg.mozilla.org/mozilla-central/rev/bb5ca84bd660
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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
Attachment #8828056 - Flags: approval-mozilla-aurora?
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.
Attachment #8828056 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Andrei, can we get some QA help with this? Michael can provide any details you may need.
Flags: needinfo?(andrei.vaida)
Depends on: 1338241
(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.
Flags: needinfo?(michael)
Attached file 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.
Flags: needinfo?(michael)
I should also add that this is the kinda-specification of the feature: https://gist.github.com/mystor/5739e222e398efc6c29108be55eb6fe3
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?
Flags: needinfo?(michael)
(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
Flags: needinfo?(michael)
[Test Plan]:
https://wiki.mozilla.org/QA/Large-Allocation_header
Flags: needinfo?(andrei.vaida)
QA Contact: ciprian.georgiu
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!
Keywords: dev-doc-needed → dev-doc-complete
(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!
(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
Blocks: 1508306
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.