1400886 - Enabled shared global for JSMs on Android

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement, P2)

Description

[Andrew McCreight [:mccr8]]

kmag disabled JSM global sharing in bug 1400489, with the comment "Android currently passes cross-JSM syntactic scope objects, which don't fare very well".

When it initially landed enabled for Android, there were some pretty big perf improvements, so it would probably be worth somebody's time to track down the remaining issues, and then reenable this on Android (just backing out bug 1400489 should suffice).

(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #16)
> 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 1

[Andrew McCreight [:mccr8]]

Kris, could you write up a bit of what the problems you saw on Android were, to help anybody who might want to work on this? Thanks.

Comment 2

[Ted Campbell [:tcampbell]]

From our discussion on #developers a few days ago (with permalinks updated):

> John-Galt> tcampbell: Hm. You remember when I said that thing about not passing context objects objects for the subscript loader from a JSM to a helper that calls the subscript loader on it? Well... http://searchfox.org/mozilla-central/rev/05c4c3bc0cfb9b0fc66bdfc8c47cac674e45f151/mobile/android/modules/geckoview/GeckoViewUtils.jsm#38 http://searchfox.org/mozilla-central/rev/05c4c3bc0cfb9b0fc66bdfc8c47cac674e45f151/mobile/android/components/BrowserCLH.js#111
> <John-Galt> Which, as it happens, landed today

There is certainly some XPC cleanup and sanity checks that should be done, but the immediate Android problem is actually that we call XPCOMUtils.defineLazyScriptGetter(scope, name, script); with an object for scope instead of a JSM |this|. This is no longer supported since it would require CCWs on all objects so that we knew which JSM they came from. The result of violating this, is that the script you bound as a getter will be added in the XPCOMUtils scope and thus unable to access your JSM globals.

(On a related note, we should probably audit this list https://searchfox.org/mozilla-central/search?q=symbol:XPCOMUtils%23defineLazyGetter&redirect=false and make no similar issues slipped through)

Comment 3

[Ted Campbell [:tcampbell]]

(In reply to Ted Campbell [:tcampbell] from comment #2)

> (On a related note, we should probably audit this list
> https://searchfox.org/mozilla-central/search?q=symbol:
> XPCOMUtils%23defineLazyGetter&redirect=false and make no similar issues
> slipped through)

Oops, that wass nonsense.. |defineLazyGetter| is fine anywhere, it is only |defineLazyScriptGetter| that is a problem, and it is used by GeckoViewUtils.jsm.

Roughly, if loadSubscript (which defineLazyScriptGetter calls), is called on a local object from a different JSM we will likely have trouble. This might preclude using defineLazyScriptGetter at all for non-global/JSMEnvironment targets.

Comment 4

[Andrew McCreight [:mccr8]]

Jim, any chance you could take a look at this? It seems like you've looked at the files involved with GeckoViewUtils.addLazyGetter recently.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Ted Campbell [:tcampbell] from comment #2)
> From our discussion on #developers a few days ago (with permalinks updated):
> 
> There is certainly some XPC cleanup and sanity checks that should be done,
> but the immediate Android problem is actually that we call
> XPCOMUtils.defineLazyScriptGetter(scope, name, script); with an object for
> scope instead of a JSM |this|. This is no longer supported since it would
> require CCWs on all objects so that we knew which JSM they came from. The
> result of violating this, is that the script you bound as a getter will be
> added in the XPCOMUtils scope and thus unable to access your JSM globals.

So would it be enough if we used a global for scope instead of an object? Right now the BrowserCLH.js component passes an object into GeckoViewUtils.addLazyGetter when loading subscripts. Would it be enough if we used BrowserCLH's |this| object instead?

Comment 6

[Kris Maglione [:kmag]]

Yes, that should be enough. It would probably be best if you just loaded it as a JSM, though.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1400886 - Change BrowserCLH subscripts into modules;

Change the subscripts (e.g. FormAssistant.js) that we load in BrowserCLH
into proper .jsm modules. This avoids the `defineLazyScriptGetter`
incompatibility mentioned in the bug, and when we turn on shared JSM
global, any memory advantage we get from using subscripts should not
matter anymore.

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

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8909608 [details]
Bug 1400886 - Change BrowserCLH subscripts into modules;

https://reviewboard.mozilla.org/r/181084/#review186514

Comment 9

[Andrew McCreight [:mccr8]]

You might as well roll the backout of bug 1400489 into this patch, too.

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8909608 [details]
Bug 1400886 - Change BrowserCLH subscripts into modules;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181084/diff/1-2/

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1400886 - Re-enable JSM shared global on mobile;

Back out the change made in bug 1400489, to re-enable shared global for
JSMs on mobile. r=me for trivial patch.

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

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8909608 [details]
Bug 1400886 - Change BrowserCLH subscripts into modules;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181084/diff/2-3/

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8909929 [details]
Bug 1400886 - Re-enable JSM shared global on mobile;

https://reviewboard.mozilla.org/r/181412/#review186672

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4edef41c7f6c
Change BrowserCLH subscripts into modules; r=snorp
https://hg.mozilla.org/integration/autoland/rev/cf450ba78f02
Re-enable JSM shared global on mobile; r=jchen

Comment 15

[Ionuț Goldan [:igoldan]]

This successfully brought back the big improvements on Android! Very nice work!

== Change summary for alert #9542 (as of September 19 2017 19:01 UTC) ==

Improvements:

 28%  remote-blank summary android-4-4-armv7-api16 opt      1,115.80 -> 801.94
 20%  remote-blank summary android-7-1-armv8-api16 opt      628.97 -> 504.37
 18%  remote-blank summary android-4-2-armv7-api16 opt      1,545.68 -> 1,263.31
 16%  remote-twitter summary android-4-4-armv7-api16 opt    1,451.91 -> 1,214.19
 14%  remote-twitter summary android-7-1-armv8-api16 opt    948.55 -> 813.24
 13%  remote-nytimes summary android-4-4-armv7-api16 opt    2,223.12 -> 1,938.53
 12%  remote-nytimes summary android-7-1-armv8-api16 opt    1,319.51 -> 1,167.57

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9542

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/4edef41c7f6c
https://hg.mozilla.org/mozilla-central/rev/cf450ba78f02

Comment 17

[Jan Henning [:JanH]]

Memory usage also seems to have dropped by about 10? MB:
https://treeherder.mozilla.org/perf.html#/graphs?timerange=31536000&series=autoland,1543160,1,4&series=autoland,1545819,1,4&zoom=1505686208270.0247,1505895099000,170000000,209033457.24907064

Comment 18

[Ionuț Goldan [:igoldan]]

Jan, you are right:

== Change summary for alert #9586 (as of September 21 2017 07:31 UTC) ==

Improvements:

  5%  Resident Memory summary android-4-2-x86 opt      198,804,364.17 -> 189,414,479.92
  4%  Resident Memory summary android-4-3-armv7-api16 opt 195,113,144.17 -> 186,858,932.00
  4%  Resident Memory summary android-api-16-gradle opt 193,589,918.83 -> 186,666,706.58

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9586