Enabled shared global for JSMs on Android

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mccr8, Assigned: jchen)

Tracking

(Blocks 1 bug)

unspecified
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec+, firefox55 wontfix, firefox56 wontfix, firefox57 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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
(Reporter)

Comment 1

2 years ago
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.
Flags: needinfo?(kmaglione+bmo)
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)
Flags: needinfo?(kmaglione+bmo)
(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.
(Reporter)

Comment 4

2 years ago
Jim, any chance you could take a look at this? It seems like you've looked at the files involved with GeckoViewUtils.addLazyGetter recently.
Flags: needinfo?(nchen)
(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?
Flags: needinfo?(nchen) → needinfo?(tcampbell)
Yes, that should be enough. It would probably be best if you just loaded it as a JSM, though.
Flags: needinfo?(tcampbell)
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment on attachment 8909608 [details]
Bug 1400886 - Change BrowserCLH subscripts into modules;

https://reviewboard.mozilla.org/r/181084/#review186514
Attachment #8909608 - Flags: review?(snorp) → review+
(Reporter)

Comment 9

2 years ago
You might as well roll the backout of bug 1400489 into this patch, too.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

2 years ago
mozreview-review
Comment on attachment 8909929 [details]
Bug 1400886 - Re-enable JSM shared global on mobile;

https://reviewboard.mozilla.org/r/181412/#review186672
Attachment #8909929 - Flags: review+

Comment 14

2 years ago
Pushed by nchen@mozilla.com:
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
tracking-fennec: --- → ?
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
https://hg.mozilla.org/mozilla-central/rev/4edef41c7f6c
https://hg.mozilla.org/mozilla-central/rev/cf450ba78f02
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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
tracking-fennec: ? → +
Priority: -- → P2
You need to log in before you can comment on or make changes to this bug.