Changes to JS components/JSMs

[Kyle Huey]

(There's a tl;dr at the bottom if you don't want to read the whole thing).

Hello all,

For B2G we have found the memory overhead of a compartment for each JSM/JS
component to be too high for the device. We have explored various
techniques to lower the overhead of a compartment, but unfortunately those
fixes that would help at all are either too risky for or cannot be
completed in time for Gecko 18. We have decided instead to consolidate all
JSMs and JS components into a single compartment in B2G.

Because of architectural changes to the JS engine since
compartment-per-global landed, that means that all JSMs and JS components
will share a single global object. Currently component and module loading
works by creating a new global for each script and executing the script
against that global. Constructs like 'var', 'let', 'const', and
'function', when executed at global scope, set properties on the global
object that the JS component loader can grab. When the pref that we've
added is set, we will use the same global for every script and instead
execute the script as a function with a 'this' object unique to each
script. At function scope, 'var', 'let', 'const', and 'function' will not
create properties that are accessible outside the function. To work around
this, we have changed to set properties via assignment on the 'this'
object. At global scope this has essentially the same effect as the
existing setup, and at function scope we can now access the properties
after the function has executed.

Executing the scripts as functions avoids most of the ways they would
interfere with each other. It does not, however, fix bareword assignments
(which will still create global properties) or things like mucking with
Object.prototype. Fortunately these issues appear to be quite rare in our
code. We decided to make these changes globally, even though we will not
be turning this on for Firefox, to avoid trying to determine exactly what
is used in B2G and what is not.

tl;dr: Code in Gecko must now set "magic" properties (such as
EXPORTED_SYMBOLS, the symbols themselves, and NSGetFactory) on the 'this'
object instead of implicitly on the global via 'var', 'let', 'function',
'const', etc

So:

const EXPORTED_SYMBOLS = ["Foo", "Bar"];
let Foo = 3;
function Bar() { dump("hi"); }
var NSGetModule = ...;

becomes

this.EXPORTED_SYMBOLS = ["Foo", "Bar"];
this.Foo = 3;
this.Bar = function Bar() { dump("hi"); }
this.NSGetModule = ...;

- Kyle

[Gregory Szorc]

On 10/30/12 9:11 AM, Kyle Huey wrote:
> tl;dr: Code in Gecko must now set "magic" properties (such as
> EXPORTED_SYMBOLS, the symbols themselves, and NSGetFactory) on the 'this'
> object instead of implicitly on the global via 'var', 'let', 'function',
> 'const', etc
>
> So:
>
> const EXPORTED_SYMBOLS = ["Foo", "Bar"];
> let Foo = 3;
> function Bar() { dump("hi"); }
> var NSGetModule = ...;
>
> becomes
>
> this.EXPORTED_SYMBOLS = ["Foo", "Bar"];
> this.Foo = 3;
> this.Bar = function Bar() { dump("hi"); }
> this.NSGetModule = ...;

To clarify, you only need to "this qualify" the symbols you wish to
export from the file. If you have file-level symbols that don't need
exported, you do *not* need to add them to "this".

So:

const Cu = Components.utils;
const EXPORTED_SYMBOLS = ["ExportedFunction"];

function ExportedFunction() { }

function supportFunction() { }

becomes

const Cu = Components.utils
this.EXPORTED_SYMBOLS = ["ExportedFunction"];

this.ExportedFunction = function ExportedFunction() { }

function supportFunction() { }

(Note that "Cu" and "supportFunction" remain unchanged.)

Also, an unfortunate side-effect of this is that you lose the ability to
easily create constant exported symbols via "const." Instead, you'll
have to do something like:

Object.defineProperty(this, "MY_CONSTANT",
{configurable: false, writable:false, value: "MY_CONSTANT_VALUE"});

p.s. khuey deserves credit for clarifying this over IRC

[Kyle Huey]

And just to be extra clear, this doesn't affect addons at all. You can
sleep easy tonight.

- Kyle

[Ehsan Akhgari]

Are the existing JS modules going to be rewritten whole-sale? Is there
a bug tracking this?

Thanks!
Ehsan

[Kyle Huey]

On Tue, Oct 30, 2012 at 10:57 AM, Ehsan Akhgari <ehsan....@gmail.com>wrote:

> Are the existing JS modules going to be rewritten whole-sale? Is there a
> bug tracking this?
>

The patch in Bug 798491 (I thought I mentioned the bug # originally) does
this.

- Kyle

[Andrew McCreight]

Kyle has already done that. Just check out the 655k patch in bug 798491.

Andrew

----- Original Message -----
> Are the existing JS modules going to be rewritten whole-sale? Is
> there
> a bug tracking this?
>

> Thanks!
> Ehsan
> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

[Alex Keybl]

> We have explored various
> techniques to lower the overhead of a compartment, but unfortunately those
> fixes that would help at all are either too risky for or cannot be
> completed in time for Gecko 18. We have decided instead to consolidate all
> JSMs and JS components into a single compartment in B2G.

In case people missed this part specifically, I just want to point out that the change in bug 798491 are targeted for FF18 (Aurora). See comment 75 for philikon's early risk evaluation. From our read of the bug, although this represents major code change, we do not expect major desktop/Android regressions, especially with a day or two of bake time on m-c to start.

-Alex

> tl;dr: Code in Gecko must now set "magic" properties (such as
> EXPORTED_SYMBOLS, the symbols themselves, and NSGetFactory) on the 'this'
> object instead of implicitly on the global via 'var', 'let', 'function',
> 'const', etc
>
> So:
>
> const EXPORTED_SYMBOLS = ["Foo", "Bar"];
> let Foo = 3;
> function Bar() { dump("hi"); }
> var NSGetModule = ...;
>
> becomes
>
> this.EXPORTED_SYMBOLS = ["Foo", "Bar"];
> this.Foo = 3;
> this.Bar = function Bar() { dump("hi"); }
> this.NSGetModule = ...;
>

> - Kyle

[Joshua Cranmer]

On 10/30/2012 12:53 PM, Kyle Huey wrote:
> And just to be extra clear, this doesn't affect addons at all. You can
> sleep easy tonight.
>

Seeing as how addons can create their own JSMs, how does this not affect
addons?

[Philipp von Weitershausen]

Because Firefox and all other products apart from B2G don't have the
"jsloader.reuseGlobal" pref enabled, so they still get a new global
per XPCOM JS file / JSM, which means a global variable is equivalent
to defining a member on `this`.

Now, maybe Fennec might want to flip this pref as well (it would make
a lot of sense IMHO) in which case we might want to re-evaluate how
much this would affect Fennec add-ons (IIRC they're all JetPack-based,
no?). Desktop Firefox is probably better served by waiting for
forthcoming optimizations from the JS team.

[Dave Townsend]

No, they aren't all Jetpack based. And even then, Jetpack uses a lot of
compartments as every module is loaded in a new global. We used to merge
all of them into the same compartment but can't do that anymore.

[Dave Townsend]

On 10/30/12 12:08, Alex Keybl wrote:
>> We have explored various
>> techniques to lower the overhead of a compartment, but unfortunately those
>> fixes that would help at all are either too risky for or cannot be
>> completed in time for Gecko 18. We have decided instead to consolidate all
>> JSMs and JS components into a single compartment in B2G.
>
>
> In case people missed this part specifically, I just want to point out that the change in bug 798491 are targeted for FF18 (Aurora). See comment 75 for philikon's early risk evaluation. From our read of the bug, although this represents major code change, we do not expect major desktop/Android regressions, especially with a day or two of bake time on m-c to start.
>
> -Alex

This plan really worries me. I agree that it is unlikely we'll see
desktop/android regressions. I'm more worried that we'll just break the
platform b2g runs on in subtle ways that we might not notice before
shipping. It's pretty concerning to have a fundamental change to how
components and modules are loaded on a new platform where to my
knowledge we don't yet have good automated testing (please please
someone prove me wrong on this).

[Justin Lebar]

On Tue, Oct 30, 2012 at 4:31 PM, Dave Townsend <dtow...@mozilla.com> wrote:
> This plan really worries me. [...] I'm worried that we'll just break the

> platform b2g runs on in subtle ways that we might not notice before shipping.
> It's pretty concerning to have a fundamental change to how components and
> modules are loaded on a new platform where to my knowledge we don't yet have
> good automated testing (please please someone prove me wrong on this).

I think we all share your concern; you'll see a fair bit of discussion
about this in the bug.

But this change gets a huge memory win on a highly-memory-constrained
device, and we don't have an alternative which would be ready in time.

https://bugzilla.mozilla.org/show_bug.cgi?id=798491

[Steve Fink]

On 10/30/2012 09:11 AM, Kyle Huey wrote:
> tl;dr: Code in Gecko must now set "magic" properties (such as
> EXPORTED_SYMBOLS, the symbols themselves, and NSGetFactory) on the 'this'
> object instead of implicitly on the global via 'var', 'let', 'function',
> 'const', etc

Is the intent to revert these change once the JS platform and release
schedule allows it? My minimal understanding of JS semantics says that
you can't have the combination of this-qualification + strict mode +
separate globals. (Also unavailable: this-qualification + sane code. But
the memory win is totally worth it.)

[Kartikaya Gupta]

On 12-10-30 16:31 , Dave Townsend wrote:
>> In case people missed this part specifically, I just want to point out
>> that the change in bug 798491 are targeted for FF18 (Aurora). See
>> comment 75 for philikon's early risk evaluation. From our read of the
>> bug, although this represents major code change, we do not expect
>> major desktop/Android regressions, especially with a day or two of
>> bake time on m-c to start.
>>
>> -Alex
>
> This plan really worries me. I agree that it is unlikely we'll see
> desktop/android regressions. I'm more worried that we'll just break the
> platform b2g runs on in subtle ways that we might not notice before
> shipping. It's pretty concerning to have a fundamental change to how
> components and modules are loaded on a new platform where to my
> knowledge we don't yet have good automated testing (please please
> someone prove me wrong on this).
>

Would it help if we turned this on for Android in nightly builds? That
would increase the chance of catching subtle breakages by increasing
user testing coverage of (some of) the code.

kats

[Dave Townsend]

My understanding is that this would break add-ons that include their own
modules and components.

[Dave Townsend]

Though yes, apart from that problem it'd certainly give me more
confidence that we have good tests in something closer to the b2g
configuration.

[Alex Keybl]

Yeah, my take is that at this point the desire to improve B2G performance (comment 61) >> the risk of subtle breakage, especially given the number of people with eyes on B2G v1 ahead of release. If anybody's got specific areas that we can QA on the phone, please do comment in the bug and we can get a QA Contact assigned. Worst case scenarios for B2G post-release would be helpful as well.

-Alex

On Oct 30, 2012, at 1:41 PM, Justin Lebar <justin...@gmail.com> wrote:

> On Tue, Oct 30, 2012 at 4:31 PM, Dave Townsend <dtow...@mozilla.com> wrote:

>> This plan really worries me. [...] I'm worried that we'll just break the

>> platform b2g runs on in subtle ways that we might not notice before shipping.
>> It's pretty concerning to have a fundamental change to how components and
>> modules are loaded on a new platform where to my knowledge we don't yet have
>> good automated testing (please please someone prove me wrong on this).
>

> I think we all share your concern; you'll see a fair bit of discussion
> about this in the bug.
>
> But this change gets a huge memory win on a highly-memory-constrained
> device, and we don't have an alternative which would be ready in time.
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=798491
>
> On Tue, Oct 30, 2012 at 4:31 PM, Dave Townsend <dtow...@mozilla.com> wrote:

[Blair McBride]

Does this break the usage of the BackstagePass object? (the thing that
Cu.import() returns).

We rely on that for various testing. Looking at MXR, it looks like the
following test areas would be affected:
* Add-ons Manager
* Sync
* Social API (toolkit)
* Desktop browser

The Add-ons Manager is shipped on B2G, but currently only used for
bootstrapping test frameworks. The rest (AFAIK) don't apply to B2G, but
some do certainly apply to Android - and we need to keep that test
coverage there.


- Blair

[Kyle Huey]

On Tue, Oct 30, 2012 at 8:02 PM, Blair McBride <bmcb...@mozilla.com> wrote:

> Does this break the usage of the BackstagePass object? (the thing that
> Cu.import() returns).
>

Not if the pref isn't set. If the pref is set I suspect it still returns
an object with the relevant properties, but that object is no longer a
BackstagePass. I haven't verified that though.

- Kyle

[Gavin Sharp]

On Thu, Nov 1, 2012 at 10:00 AM, Kyle Huey <m...@kylehuey.com> wrote:
> Not if the pref isn't set. If the pref is set I suspect it still returns
> an object with the relevant properties, but that object is no longer a
> BackstagePass. I haven't verified that though.

Will that break
http://mxr.mozilla.org/mozilla-central/source/toolkit/addon-sdk/loader.js#11
or http://mxr.mozilla.org/mozilla-central/source/toolkit/addon-sdk/promise/core.js#10
? Looks like the patch didn't update them...

Gavin

[Kyle Huey]

It would if we turned this on for a product that uses Jetpack, but we don't
currently have plans to do that.

- Kyle

[Gregory Szorc]

I am writing chrome code that Cu.imports core.js from promises. It will likely need to be backported to Aurora/18 to run on B2G. This can't break.

[David Rajchenbach-Teller]

On 11/2/12 4:42 PM, Kyle Huey wrote:
> On Thu, Nov 1, 2012 at 10:24 AM, Gavin Sharp <ga...@gavinsharp.com> wrote:
>
>> On Thu, Nov 1, 2012 at 10:00 AM, Kyle Huey <m...@kylehuey.com> wrote:
>>> Not if the pref isn't set. If the pref is set I suspect it still returns
>>> an object with the relevant properties, but that object is no longer a
>>> BackstagePass. I haven't verified that though.
>>
>> Will that break
>>
>> http://mxr.mozilla.org/mozilla-central/source/toolkit/addon-sdk/loader.js#11
>> or
>> http://mxr.mozilla.org/mozilla-central/source/toolkit/addon-sdk/promise/core.js#10
>> ? Looks like the patch didn't update them...
>>
>>
> It would if we turned this on for a product that uses Jetpack, but we don't
> currently have plans to do that.

We actually have some code that imports these without using Jetpack,
including OS.File (although OS.File doesn't work on Gonk, though).

Cheers,
David

--
David Rajchenbach-Teller, PhD
Performance Team, Mozilla

[Gavin Sharp]

Right - the linked modules are not jetpack-only code - they're part of
"core" gecko code now, and the plan is to land even more of them, so
we need to keep them working somehow.

Gavin

[Mook]

Ping - was this part ever answered? I've written (external) things that
depended on BackstagePass being available (due to things like bug
805687); that's definitely importing from the platform.

Of course, I'm not touching B2G/Fennec yet, so I'm okay... but that's
exposed API already ;)

--
Mook

[Kyle Huey]

On Sun, Nov 4, 2012 at 11:53 PM, Mook
<mook.moz+nntp.n...@gmail.com.please-avoid-direct-mail>
wrote:

> Ping - was this part ever answered?

Did you see my email on November 1st?

- Kyle

[Mook]

Argh; sorry, Thunderbird screwed up threading there and decided your
message was a sibling. That, and I initially read it as "it'll work
until we decide to change it", and there's been a... history of prefs
being flipped because it's easy to do so and explicitly not supporting
external code that depended on the old behaviour :(

--
Mook