Closed Bug 1400886 Opened 3 years ago Closed 3 years ago

Enabled shared global for JSMs on Android

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
fennec + ---
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: mccr8, Assigned: jchen)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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
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.
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 on attachment 8909608 [details]
Bug 1400886 - Change BrowserCLH subscripts into modules;

https://reviewboard.mozilla.org/r/181084/#review186514
Attachment #8909608 - Flags: review?(snorp) → review+
Blocks: 1386631
You might as well roll the backout of bug 1400489 into this patch, too.
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+
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
Closed: 3 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.