1381961 - Enabled shared global for JSMs pref

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Description

[Andrew McCreight [:mccr8]]

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 1

[Andrew McCreight [:mccr8]]

Attachment: Bug 1381961 - Enabled shared global for JSMs pref.

Review commit: https://reviewboard.mozilla.org/r/176496/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/176496/

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8904706 [details]
Bug 1381961 - Enabled shared global for JSMs pref.

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

Comment 3

[Jean Gong :jgong]

This bug won't be fix by 57; moving it to P2 for post 57 work.

Comment 4

[Kris Maglione [:kmag]]

I just pushed the parts of bug 1399997 that were blocking this, so I think this is ready to land (on inbound).

Comment 5

[Ted Campbell [:tcampbell]]

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.

Comment 6

[Andrew McCreight [:mccr8]]

try is still green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=582caf274b691d119d201187d49a546e35bc6ba5

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dd653d5758d5
Enabled shared global for JSMs pref. r=kmag,tcampbell

Comment 8

[Ted Campbell [: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.

Comment 9

[Kris Maglione [:kmag]]

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

Comment 10

[Kris Maglione [:kmag]]

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

Comment 11

[David Bruant]

(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

Comment 12

[Kris Maglione [:kmag]]

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

Comment 13

[Kris Maglione [:kmag]]

Attachment: 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.

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 15

[Tom S [:evilpie]]

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

Comment 16

[Ionuț Goldan [:igoldan]]

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

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Do we have a follow-up bug to get this enabled on android?

Comment 18

[Andrew McCreight [:mccr8]]

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

Comment 19

[Ionuț Goldan [:igoldan]]

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