Enabled shared global for JSMs pref

RESOLVED FIXED in Firefox 57

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Depends on 1 bug, Blocks 2 bugs)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57 fixed)

Details

(Whiteboard: [MemShrink:P1][qf:p2])

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
There are enough dicey looking changes to the JS engine that I'd like to land the code in bug 1186409, wait a few days for it to stability, then flip on the pref in this bug.
Comment hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
Comment on attachment 8904706 [details]
Bug 1381961 - Enabled shared global for JSMs pref.

https://reviewboard.mozilla.org/r/176496/#review181586
Attachment #8904706 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Updated

2 years ago
Depends on: 1396050, 1396145, 1381965
Whiteboard: [MemShrink:P1][qf:p1]
(Assignee)

Updated

2 years ago
Depends on: 1398382
Depends on: 1398601

Comment 3

2 years ago
This bug won't be fix by 57; moving it to P2 for post 57 work.
Whiteboard: [MemShrink:P1][qf:p1] → [MemShrink:P1][qf:p2]
(Assignee)

Updated

2 years ago
Depends on: 1398895
Depends on: 1399960
Depends on: 1399997
I just pushed the parts of bug 1399997 that were blocking this, so I think this is ready to land (on inbound).
(Assignee)

Updated

2 years ago
No longer depends on: 1396145
(Assignee)

Updated

2 years ago
Attachment #8904706 - Flags: review?(tcampbell)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8904706 [details]
Bug 1381961 - Enabled shared global for JSMs pref.

https://reviewboard.mozilla.org/r/176496/#review185524

From a SpiderMonkey perspective, JSM sharing is very similar to the current FrameScript loader behavior. One difference is the target of |this| is different in those cases. The edges around this difference have been fixed up in SpiderMonkey. The primary changes for this work in SpiderMonkey relate to the XPConnect / JSAPI interface. Iterations have been done to clean-up and test this interface over last few weeks and it seems to be stablized.
Attachment #8904706 - Flags: review?(tcampbell) → review+

Comment 7

2 years ago
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd653d5758d5
Enabled shared global for JSMs pref. r=kmag,tcampbell
Notes to JSM Authors
--------------------

Your global vars, lets, consts, and |this| will now belong to a scope within a shared JS global. JSMs will behave 99% unchanged.

The |(1,eval)(...)| and |new Function(...)| patterns now run in the context of the shared global and are thus DEPRECATED. In future it is likely that CSP will be used to deny all eval/Function usage.

The shared global namespace will become become polluted by Cu.importGlobalProperties and WebIDL classes. If you need one of these names, assume it isn't available and continue to load it normally. If you define a variable in your JSM with the same name, yours will cover and so you shouldn't worry.

Functions such as Cu.getGlobalForObject(...) will return the shared global and should primarily only be used for compartment-related matters.

In the devtools debugger, you may see a scope labelled |NonSyntacticVariablesObject|. This is the per-JSM environment and the BackstagePass is the shared global.

There is an environment variable MOZ_LOADER_SHARE_GLOBAL=0/1 to control enable/disable shared global support. This can be useful in certain testing scenarios. The jsloader.shareGlobal pref exists in about:config as well and only takes effect on restart.
(In reply to Ted Campbell [:tcampbell] from comment #8)
> Notes to JSM Authors
> --------------------

Also:

You now share prototypes and constructors for built-in objects like Object, Array, Function, ... with all other system JSMs. They don't belong solely to you anymore. Do not add properties to them, or modify them in any other way.

Beware using the subscript loader to load scripts into objects from other JSMs. They'll be executed in the context of your pseudo-global scope. Also beware of creating wrappers around the subscript loader that take a non-global scope object, for the same reason. Their scopes will be determined by the location of the loadSubScript call.
Fun fact: This almost exactly cancels out the responsiveness regression caused by adding a typical amount of overhead to our talos test WebExtension in bug 1398974:

https://treeherder.mozilla.org/perf.html#/graphs?series=autoland,1493048,1,1
(Assignee)

Updated

2 years ago
Priority: -- → P1

Comment 11

2 years ago
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #9)
> (In reply to Ted Campbell [:tcampbell] from comment #8)
> > Notes to JSM Authors
> > --------------------
> 
> Also:
> 
> You now share prototypes and constructors for built-in objects like Object,
> Array, Function, ... with all other system JSMs. They don't belong solely to
> you anymore. Do not add properties to them, or modify them in any other way.

One way to enforce this even to JSM authors who won't read this message would be to freeze or tamper-proof (as described in [1]) the built-ins.

If a runtime solution is off-table, maybe an ESLint rule can be used to help patch reviewers catch these sorts of problems 

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1186409#c96
(In reply to David Bruant from comment #11)
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #9)
> > You now share prototypes and constructors for built-in objects like Object,
> > Array, Function, ... with all other system JSMs. They don't belong solely to
> > you anymore. Do not add properties to them, or modify them in any other way.
> 
> One way to enforce this even to JSM authors who won't read this message
> would be to freeze or tamper-proof (as described in [1]) the built-ins.

I have thought about that, yeah. We can probably do it for the most common cases as a follow-up, in the next cycle.

The main problem is that we create a ton of WebIDL prototypes lazily, so we can't freeze them when the global is created without a massive startup performance regression. We might be able to handle them from the global's resolve hook, but that might be expensive, and could make it more difficult to migrate BackstagePass to WebIDL.
The 90 day ts_paint graph right now makes for a pretty nice visual. That's the sharpest drop I've ever seen.
Depends on: 1400489
https://hg.mozilla.org/mozilla-central/rev/dd653d5758d5
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #13)
> Created attachment 8908927 [details]
> 90 day ts_paint graph
> 
> The 90 day ts_paint graph right now makes for a pretty nice visual. That's
> the sharpest drop I've ever seen.

This is honestly quite amazing. Great work.
Before this was disabled on Android by bug 1400489, these improvements were noticed:

== Change summary for alert #9484 (as of September 15 2017 18:25 UTC) ==

Improvements:

 18%  remote-blank summary android-4-2-armv7-api16 opt      1,546.13 -> 1,264.19
 15%  remote-twitter summary android-4-4-armv7-api16 opt    1,434.59 -> 1,212.95
 15%  remote-nytimes summary android-4-4-armv7-api16 opt    2,237.48 -> 1,895.33
 13%  remote-twitter summary android-7-1-armv8-api16 opt    937.82 -> 811.54
 12%  remote-nytimes summary android-7-1-armv8-api16 opt    1,317.97 -> 1,164.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9484
Do we have a follow-up bug to get this enabled on android?
(Assignee)

Updated

2 years ago
Blocks: 1400886
(Assignee)

Comment 18

2 years ago
(In reply to Ben Kelly [:bkelly] from comment #17)
> Do we have a follow-up bug to get this enabled on android?

I've filed bug 1400886.
This brought tons of improvements from AWSY's side! Great work!

== Change summary for alert #9496 (as of September 15 2017 18:25 UTC) ==

Improvements:

 17%  JS summary macosx64-stylo-disabled opt stylo-disabled     114,555,014.10 -> 95,477,123.41
 16%  JS summary linux64-stylo-sequential opt stylo-sequential  114,599,722.21 -> 95,796,821.72
 16%  JS summary windows10-64-stylo-disabled opt stylo-disabled 126,208,265.10 -> 105,653,755.54
 16%  JS summary windows10-64 opt                               126,175,914.82 -> 105,817,503.87
 16%  JS summary linux64 opt                                    114,355,945.41 -> 95,946,156.84
 16%  JS summary windows10-64 pgo                               125,728,792.97 -> 105,707,020.88
 16%  JS summary linux64-stylo-disabled opt stylo-disabled      113,618,511.57 -> 95,840,888.00
 16%  JS summary osx-10-10 opt                                  114,245,833.46 -> 96,387,533.92
 14%  JS summary windows7-32 opt                                92,647,884.62 -> 79,622,919.25
 14%  JS summary linux32-stylo-disabled opt stylo-disabled      82,084,631.58 -> 70,554,721.11
 13%  JS summary windows7-32-stylo-disabled opt stylo-disabled  91,851,303.87 -> 79,733,426.07
 13%  JS summary windows7-32 pgo                                91,940,592.80 -> 80,212,875.15
 10%  Explicit Memory summary windows10-64-stylo-disabled opt stylo-disabled299,032,631.57 -> 270,313,140.57
 10%  Explicit Memory summary windows10-64 opt                  306,542,385.89 -> 277,346,581.85
 10%  Explicit Memory summary windows10-64 pgo                  305,180,368.82 -> 276,148,169.42
  9%  Explicit Memory summary linux64-stylo-sequential opt stylo-sequential302,205,345.74 -> 275,931,741.84
  8%  Explicit Memory summary linux64 opt                       303,371,903.85 -> 277,858,311.52
  8%  Explicit Memory summary windows7-32 opt                   235,877,500.52 -> 216,280,948.26
  8%  Explicit Memory summary osx-10-10 opt                     317,421,264.38 -> 291,791,502.69
  8%  Explicit Memory summary linux64-stylo-disabled opt stylo-disabled295,313,193.80 -> 271,815,831.14
  8%  Explicit Memory summary macosx64-stylo-disabled opt stylo-disabled310,315,442.53 -> 285,935,740.64
  8%  Explicit Memory summary windows7-32 pgo                   234,946,697.13 -> 216,623,489.80
  8%  Resident Memory summary linux64-stylo-disabled opt stylo-disabled441,578,816.51 -> 407,942,697.04
  7%  Explicit Memory summary windows7-32-stylo-disabled opt stylo-disabled227,849,952.94 -> 211,227,079.13
  7%  Resident Memory summary linux64 opt                       455,435,331.95 -> 422,332,676.74
  7%  Explicit Memory summary linux32-stylo-disabled opt stylo-disabled224,418,258.71 -> 208,442,997.18
  7%  Resident Memory summary linux64-stylo-sequential opt stylo-sequential446,143,486.17 -> 414,715,203.52
  7%  Resident Memory summary windows10-64 pgo                  455,250,405.40 -> 424,952,324.55
  7%  Resident Memory summary windows10-64 opt                  467,285,079.09 -> 436,898,718.12
  6%  Resident Memory summary windows10-64-stylo-disabled opt stylo-disabled456,428,393.96 -> 427,427,046.81
  6%  Resident Memory summary windows7-32 pgo                   298,090,644.52 -> 280,782,555.37
  6%  Resident Memory summary osx-10-10 opt                     618,169,655.53 -> 582,918,579.39
  6%  Resident Memory summary windows7-32 opt                   307,031,666.97 -> 289,765,445.13
  5%  Resident Memory summary macosx64-stylo-disabled opt stylo-disabled617,163,519.81 -> 589,067,867.11
  3%  Heap Unclassified summary windows7-32-stylo-disabled opt stylo-disabled39,651,748.62 -> 38,487,953.77
  3%  Heap Unclassified summary windows10-64-stylo-disabled opt stylo-disabled46,834,892.81 -> 45,476,726.97
  2%  Heap Unclassified summary windows10-64 pgo                46,659,172.82 -> 45,649,656.61
  2%  Heap Unclassified summary linux64-stylo-disabled opt stylo-disabled55,058,442.10 -> 53,926,979.12
  2%  Heap Unclassified summary macosx64-stylo-disabled opt stylo-disabled69,803,596.89 -> 68,397,057.69

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9496
Depends on: 1401373
You need to log in before you can comment on or make changes to this bug.