990353 - Don't save script-sources for B2G chrome and built-in apps

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Ting-Yuan Huang]

After discussing with Luke and nbp, we decided not to save sources for chrome scripts and built-in apps on B2G as a short-term solution. This saved around 4.3MB in latest build. A drawback is toSource()/toString() on function will be fail.

A long-term solution would be to add options in application manifests to specify if the app needs the source or not.

Comment 1

[Ting-Yuan Huang]

Attachment: Don't save script-souces for B2G chrome and built-in apps

Gabor, would you mind to review my patch? It's simple and basically a subset of part.3 of the patch in bug 944659, with LOCAL_SOURCE replaced by NO_SOURCE.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8399776 [details] [diff] [review]
Don't save script-souces for B2G chrome and built-in apps

Review of attachment 8399776 [details] [diff] [review]:
-----------------------------------------------------------------

What is the improvement on a memory profile?
Is there any change in the time needed for the start-up of Gaia applications?

::: content/base/src/nsScriptLoader.cpp
@@ +1042,5 @@
> +      scheme.EqualsLiteral("jar")) {
> +    return true;
> +  }
> +
> +  if (scheme.EqualsLiteral("app")) {

Honza told me that we should not rely on the scheme to make such assumptions, is it valid to do it here?

::: dom/workers/ScriptLoader.cpp
@@ +716,5 @@
>  
>      JS::CompileOptions options(aCx);
>      options.setFileAndLine(filename.get(), 1);
> +#ifdef MOZ_WIDGET_GONK
> +    options.setSourcePolicy(JS::CompileOptions::NO_SOURCE);

Workers can be created from any web page on the web.

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8399776 [details] [diff] [review]
Don't save script-souces for B2G chrome and built-in apps

Review of attachment 8399776 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsScriptLoader.cpp
@@ +1008,5 @@
> +};
> +
> +static bool
> +IsBuiltInHost(nsCString &host)
> +{

:21, told me that you look at the principal instead of doing such matches.

Comment 4

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8399776 [details] [diff] [review]
Don't save script-souces for B2G chrome and built-in apps

Review of attachment 8399776 [details] [diff] [review]:
-----------------------------------------------------------------

I would prefer someone else reviewing this patch. Since I'm not a content/DOM peer I don't feel the right to r+ any non-trivial patch.
Passing the ball back to Bobby.

A few notes:
+/* This is a short-term solution and not cool. */

Would be nice to have some more comment that explains what this all means for Apps. What would be even better
to achieve the same goal with less low level strcmp magic if that's possible. Or maybe build this info into
the principal of the apps somehow if it's not there already?

     options.setSourcePolicy(mOutOfLine ? JS::CompileOptions::LAZY_SOURCE
                                        : JS::CompileOptions::SAVE_SOURCE);
+#ifdef MOZ_WIDGET_GONK
+    options.setSourcePolicy(JS::CompileOptions::NO_SOURCE);
+#endif

Nit: I would use #elif instead of calling setSourcePolicy twice

And most importantly I'm missing a comment that explain what this patch does and why. It comes with an unexpected behavior (toString/toSource will not work in some cases) so an explanation is important. Also, some test might makes sense just to guard against someone "fixing" it accidentally, but well placed comments might be enough.

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8399776 [details] [diff] [review]
Don't save script-souces for B2G chrome and built-in apps

Review of attachment 8399776 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I think there are a number of problems with this patch that have already been pointed out.

Comment 6

[Ting-Yuan Huang]

(In reply to Nicolas B. Pierron [:nbp] from comment #3)

> Honza told me that we should not rely on the scheme to make such
> assumptions, is it valid to do it here?

> :21, told me that you look at the principal instead of doing such matches.

More than 95% of scripts are not chrome and don't satisfy sSecurityManager->SubjectPrincipalIsSystem(). Did you mean specifying a new principal for NO_SOURCE in application manifest?

Comment 7

[Ting-Yuan Huang]

Tim, may I know your opinion, as a Gaia developer, to disallowing some APIs in built-in apps for saving memory? We are trying to make toSource()/toString() always return nothing for built-in apps.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Yeah it's fine (for functions). There is not use case I have known of for toSource(). For toString() there are few but not in Gaia.

Although we need to make the error explicit for the implicit behavior, IMHO.
(strictly speaking, we are making our JS engine violate ECMAScript spec; so we need to make our intention clear ....)

Comment 9

[Ting-Yuan Huang]

Attachment: WIP -- nosource.diff

Changed from the heuristic that checks schemes and hosts to that checks scripts with system principle or certified apps (all builtin apps are certified).

One problem remains: There are some NO_SOURCE already. After the change, when should an exception be raised? On all certified apps calling toSource() and to remain silent when called by chrome code?

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8401185 [details] [diff] [review]
WIP -- nosource.diff

Review of attachment 8401185 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsScriptLoader.cpp
@@ +1033,5 @@
>      MOZ_ASSERT(elementVal.isObject());
>      aOptions->setElement(&elementVal.toObject());
>    }
> +#ifdef MOZ_WIDGET_GONK
> +  nsCOMPtr<nsIPrincipal> principal = nsContentUtils::GetSubjectPrincipal();

You almost certainly don't want the subject principal here. I think you want nsContentUtils::GetObjectPrincipal(aScopeChain).

@@ +1036,5 @@
> +#ifdef MOZ_WIDGET_GONK
> +  nsCOMPtr<nsIPrincipal> principal = nsContentUtils::GetSubjectPrincipal();
> +  if (nsContentUtils::IsSystemPrincipal(principal) ||
> +      (principal &&
> +       principal->GetAppStatus() == nsIPrincipal::APP_STATUS_CERTIFIED)) {

Please factor this (and the MOZ_WIDGET_GONK) stuff into a method called:

nsContentUtils::MayDiscardSource(nsIPrincipal* aScopePrincipal). You can just #ifdef this to false in non-gonk builds.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

On second thought, I don't think we should be worry about the behavior here very much, since we are not exposing it to anything outside Mozilla.

Comment 12

[Joe Cheng [:jcheng] (please needinfo)]

triage: not blocking tarako release but when we have r+, let's reconsider for uplift to 1.3T thanks

Comment 13

[Ting-Yuan Huang]

Attachment: WIP -- nosource.diff v.2

(In reply to Bobby Holley (:bholley) from comment #10)
> You almost certainly don't want the subject principal here. I think you want
> nsContentUtils::GetObjectPrincipal(aScopeChain).
> 
> Please factor this (and the MOZ_WIDGET_GONK) stuff into a method called:
> 
> nsContentUtils::MayDiscardSource(nsIPrincipal* aScopePrincipal). You can
> just #ifdef this to false in non-gonk builds.

Thanks! Besides the problem you pointed out, I also refactored the worker parts.

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 8402595 [details] [diff] [review]
WIP -- nosource.diff v.2

Review of attachment 8402595 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/public/nsContentUtils.h
@@ +2047,5 @@
>    static bool IsJavascriptMIMEType(const nsAString& aMIMEType);
>  
> +  /**
> +   * If it's chrome code or a certified app, we can be sure that toSource()
> +   * will never be called.

Let's change this to say:

In some memory-constrained configurations (i.e. b2g), we discard Function source for trusted and certified script.

@@ +2050,5 @@
> +   * If it's chrome code or a certified app, we can be sure that toSource()
> +   * will never be called.
> +   * See https://bugzilla.mozilla.org/show_bug.cgi?id=990353
> +   */
> +  static bool MayDiscardSource(JS::Handle<JSObject *> aScopeChain);

I think the #ifdef MOZ_WIDGET_GONK should go in the header, so that the compiler can optimize this out in non-gonk builds. Though actually, I think maybe we can just move the whole function to the header?

::: content/base/src/nsContentUtils.cpp
@@ +6658,5 @@
>    return false;
>  }
> +
> +bool
> +nsContentUtils::MayDiscardSource(JS::Handle<JSObject *> aScopeChain)

I recognize that this call signature makes sense given the usage here, but I think that from an API-design perspective this should take an nsIPrincipal. If we inline MayDiscardSource and also mark nsContentUtils::GetObjectPrincipal |const|, the perf should be the same.

::: dom/workers/WorkerPrivate.h
@@ +1053,5 @@
> +  // If it's chrome code or a certified app, we can be sure that toSource()
> +  // will never be called.
> +  // See https://bugzilla.mozilla.org/show_bug.cgi?id=990353
> +  bool
> +  MayDiscardSource();

The same comments from the nsContentUtils version apply here.

Comment 15

[Ting-Yuan Huang]

(In reply to Bobby Holley (:bholley) from comment #14)
> > +  static bool MayDiscardSource(JS::Handle<JSObject *> aScopeChain);
> 
> I think the #ifdef MOZ_WIDGET_GONK should go in the header, so that the
> compiler can optimize this out in non-gonk builds. Though actually, I think
> maybe we can just move the whole function to the header?

Moving the whole function to the header exposes implementation details (principal->GetAppStatus() is called) and makes every .cpp files need to include nsIPrincipal.h. The other approach is to hide the details in a private static method, MayDiscardSourceImpl(), while inlining MayDiscardSource():

static bool MayDiscardSource(nsIPrincipal* principal)
{
#ifdef MOZ_WIDGET_GONK
    return MayDiscardSourceImpl(principal);
#else
    return false;
#endif
}

But I'm not sure which one is better. The above or to include nsIPrincipal.h in nsContentUtils.h?

> ::: content/base/src/nsContentUtils.cpp
> @@ +6658,5 @@
> >    return false;
> >  }
> > +
> > +bool
> > +nsContentUtils::MayDiscardSource(JS::Handle<JSObject *> aScopeChain)
> 
> I recognize that this call signature makes sense given the usage here, but I
> think that from an API-design perspective this should take an nsIPrincipal.
> If we inline MayDiscardSource and also mark
> nsContentUtils::GetObjectPrincipal |const|, the perf should be the same.

I don't understand |const| here. Did you mean specifying the returned principal const so that the performance would not degrade due to aliasing?

Comment 16

[Bobby Holley (:bholley)]

(In reply to Ting-Yuan Huang from comment #15)

> Moving the whole function to the header exposes implementation details
> (principal->GetAppStatus() is called) and makes every .cpp files need to
> include nsIPrincipal.h.

Ah, right. I thought it was already included but it looks like it's not.

The other approach is to hide the details in a
> private static method, MayDiscardSourceImpl(), while inlining
> MayDiscardSource():
> 
> static bool MayDiscardSource(nsIPrincipal* principal)
> {
> #ifdef MOZ_WIDGET_GONK
>     return MayDiscardSourceImpl(principal);
> #else
>     return false;
> #endif
> }

That definitely works. But looking at this again, I think that none of these callsites are hot enough that this matters. So you're also welcome to just leave it the way you had it.

> I don't understand |const| here. Did you mean specifying the returned
> principal const so that the performance would not degrade due to aliasing?

Nevermind - Some wires got crossed in my brain again. Sorry for the noise on both points. :-(

Comment 17

[Ting-Yuan Huang]

Attachment: Discard Function source for trusted and certified script

Thanks! This is the patch ready for a formal review:

(In reply to Bobby Holley (:bholley) from comment #14)
> Comment on attachment 8402595 [details] [diff] [review]
> WIP -- nosource.diff v.2
> 
> Review of attachment 8402595 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/public/nsContentUtils.h
> @@ +2047,5 @@
> >    static bool IsJavascriptMIMEType(const nsAString& aMIMEType);
> >  
> > +  /**
> > +   * If it's chrome code or a certified app, we can be sure that toSource()
> > +   * will never be called.
> 
> Let's change this to say:
> 
> In some memory-constrained configurations (i.e. b2g), we discard Function
> source for trusted and certified script.

Thanks, this is better.

> @@ +2050,5 @@
> > +   * If it's chrome code or a certified app, we can be sure that toSource()
> > +   * will never be called.
> > +   * See https://bugzilla.mozilla.org/show_bug.cgi?id=990353
> > +   */
> > +  static bool MayDiscardSource(JS::Handle<JSObject *> aScopeChain);
> 
> I think the #ifdef MOZ_WIDGET_GONK should go in the header, so that the
> compiler can optimize this out in non-gonk builds. Though actually, I think
> maybe we can just move the whole function to the header?

Done, in the way posted in comment 15.

> ::: dom/workers/WorkerPrivate.h
> @@ +1053,5 @@
> > +  // If it's chrome code or a certified app, we can be sure that toSource()
> > +  // will never be called.
> > +  // See https://bugzilla.mozilla.org/show_bug.cgi?id=990353
> > +  bool
> > +  MayDiscardSource();
> 
> The same comments from the nsContentUtils version apply here.

Done.

Comment 18

[Bobby Holley (:bholley)]

So, I thought a bit about this, and decided that the underlying design isn't quite right. Fundamentally, we want the decision about whether or not to discard source to be per-global, not per-script. I also wanted to make this pref-controlled so that we can test this configuration on Desktop.

I didn't want to make Ting-Yuan do all that work, so I whipped up some patches on my own. I'll upload them shortly.

Comment 19

[Bobby Holley (:bholley)]

Attachment: rollup patch

Ting-Yuan, can you try out this patch and measure the memory characteristics? They should be at least as good as the other one, and possibly better, since we're covering more stuff.

Comment 20

[Bobby Holley (:bholley)]

Comment on attachment 8404973 [details] [diff] [review]
rollup patch

(note that this is a roll-up patch. The actual patch for reviewing/landing is split into many steps)

Comment 21

[Ting-Yuan Huang]

Attachment: memory-reports to attachment 8404973

It works except for ril worker, which should be the only web worker on b2g. The script-sources in worker is still ~1MB.

Attached in the memory-reports collected on ZTE-open. Let me know if more info is needed.

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Firefox OS::General is rarely the right place for a bug.

Comment 23

[Bobby Holley (:bholley)]

It turns out that UsesSystemPrincipal() returns undefined results until after we've loaded the script. I could fix this by just moving the setDiscardSource() call to WorkerPrivate::SetPrincipal, but this discussion caused bent to raise objections to discarding the source for certified apps. And now he wants to talk to sicking. So we're blocked on that.

Comment 24

[Bobby Holley (:bholley)]

We talked about this on IRC:

<sicking> bholley: if i get to wish for fairy tail and ponies, i'd ask that we do this per-script and make any loads from chrome:// and app:// drop the source
<sicking> bholley: if i don't get that, then I'd say drop the load for the system principal as well as principals for certified/privileged apps

Tracking this state per-global is much less error-prone, so I'm going to go with that.

Comment 25

[Bobby Holley (:bholley)]

Attachment: discarding-rollup-v2.patch

Ting-Yuan: this patch should fix things for the RIL worker. Can you have a look?

Comment 26

[Bobby Holley (:bholley)]

Attachment: Part 1 - Make the decision to discard source entirely per-global, rather than per-script. v1

This is effectively a policy decision based on the kind of code we expect to be
running somewhere. This is in contrast to lazy source, which is often a practical
per-script consideration of whether or not we can retrieve the source if requested.

More importantly, tracking this information on the global is much easier to
get right than tracking it on the script.

Comment 27

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add a pref for system source discarding. v1

Comment 28

[Bobby Holley (:bholley)]

Comment on attachment 8405706 [details] [diff] [review]
Part 2 - Add a pref for system source discarding. v1

Actually, I guess I was talking about this with bent, so he should review it.

Comment 29

[Bobby Holley (:bholley)]

Attachment: Part 3 - Flag for discarding where appropriate. v1

Comment 30

[Bobby Holley (:bholley)]

Attachment: Part 4 - Give tests/chrome its own moz.build file, like tests/mochitest. v1

We also add a mochitest.ini file, which will eventually be used for http-served
files that are associated with chrome mochitests.

Comment 31

[Bobby Holley (:bholley)]

Attachment: Part 5 - Tests. v1

Comment 32

[Bobby Holley (:bholley)]

Attachment: Part 6 - Flip on source discarding for b2g. v1

Comment 33

[Ting-Yuan Huang]

Comment on attachment 8405688 [details] [diff] [review]
discarding-rollup-v2.patch

Confirmed here. The result is as good as expected.

Comment 34

[Luke Wagner [:luke]]

Comment on attachment 8405705 [details] [diff] [review]
Part 1 - Make the decision to discard source entirely per-global, rather than per-script. v1

Review of attachment 8405705 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeCompiler.cpp
@@ +224,5 @@
> +    if (!cx->compartment()->options().discardSource()) {
> +        // Note that we only mark the script as having retrievable source if
> +        // the global doesn't have discardSource set. This is so that
> +        // setDiscardSource(true) has clear and consistent effects, that don't
> +        // depend on whether the source happens to be accessible via the hook.

I'm not sure this comment adds any information; I'd rather remove it.

::: js/src/shell/js.cpp
@@ +873,3 @@
>          return false;
> +    if (v.isBoolean()) {
> +        options.setSourceIsLazy(v.toBoolean());

No { }

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +821,5 @@
>          CompileOptions options(cx);
>          options.setNoScriptRval(mReuseLoaderGlobal ? false : true)
>                 .setVersion(JSVERSION_LATEST)
>                 .setFileAndLine(nativePath.get(), 1)
> +               .setSourceIsLazy(mReuseLoaderGlobal);

I don't see anywhere where the discardSource is set for the global we're loading into; I'll assume you know what you're doing here?

Comment 35

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8405706 [details] [diff] [review]
Part 2 - Add a pref for system source discarding. v1

Review of attachment 8405706 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/xpcpublic.h
@@ +472,5 @@
>  
> +// This function may be used off-main-thread, in which case it is benignly
> +// racey.
> +bool
> +ShouldDiscardSystemSource();

It will trigger race warnings at least. If you intend it to be useful off the main thread then it should be atomic.

Comment 36

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8405707 [details] [diff] [review]
Part 3 - Flag for discarding where appropriate. v1

Review of attachment 8405707 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/nsXPConnect.cpp
@@ +414,5 @@
>      }
>  
> +    if (ShouldDiscardSystemSource()) {
> +        nsIPrincipal *prin = GetObjectPrincipal(aGlobal);
> +        bool isSystem = nsContentUtils::IsSystemPrincipal(prin);

Nit: I'd quibble and call this 'shouldDiscard'

Comment 37

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8405709 [details] [diff] [review]
Part 5 - Tests. v1

Review of attachment 8405709 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

Comment 38

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8405710 [details] [diff] [review]
Part 6 - Flip on source discarding for b2g. v1

Review of attachment 8405710 [details] [diff] [review]:
-----------------------------------------------------------------

To be clear, have you tested this? I wouldn't be surprised if we have a toSource() somewhere...

Comment 39

[Bobby Holley (:bholley)]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #38)
> Comment on attachment 8405710 [details] [diff] [review]
> To be clear, have you tested this? I wouldn't be surprised if we have a
> toSource() somewhere...

Forwarding this question to Ting-Yuan.

Comment 40

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #34)
> ::: js/xpconnect/loader/mozJSComponentLoader.cpp
> @@ +821,5 @@
> >          CompileOptions options(cx);
> >          options.setNoScriptRval(mReuseLoaderGlobal ? false : true)
> >                 .setVersion(JSVERSION_LATEST)
> >                 .setFileAndLine(nativePath.get(), 1)
> > +               .setSourceIsLazy(mReuseLoaderGlobal);
> 
> I don't see anywhere where the discardSource is set for the global we're
> loading into; I'll assume you know what you're doing here?

This happens based on the principal in xpc::CreateGlobalObject.

Comment 41

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add a pref for system source discarding. v2

Comment 42

[Michael Shal [:mshal]]

Comment on attachment 8405708 [details] [diff] [review]
Part 4 - Give tests/chrome its own moz.build file, like tests/mochitest. v1

Stealing review from gps. This looks fine to me!

Comment 43

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=917f61363fd7

Comment 44

[James Ho]

OK to take this to 1.3T?

Comment 45

[Ting-Yuan Huang]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #38)
> To be clear, have you tested this? I wouldn't be surprised if we have a
> toSource() somewhere...

Confirmed with Tim weeks ago and there should be no gaia codes using toSource(). Try server also said OK with my patch. However, I grep toSource in gaia and found two js files referring to it:

./build/r.js:                optimized = String(compiler.toSource());
./test_apps/test-agent/common/vendor/sinon/sinon.js:        if (source.toSource) {
./test_apps/test-agent/common/vendor/sinon/sinon.js:            target.toSource = function toSource() {
./test_apps/test-agent/common/vendor/sinon/sinon.js:                return source.toSource();
./test_apps/test-agent/common/vendor/sinon/sinon.js:            delete target.toSource;

I'm not sure if they matters or not. @Tim, may I know your opinion?

Comment 46

[Tim Guan-tin Chien [:timdream] (please needinfo)]

That should be fine.

r.js is not run on the phone, but a compressor/assembler on build time run by xpcshell.
sinon.js is a part of our unit test framework. We run it on Fx desktop or B2G desktop and never on a phone.

Comment 47

[Ting-Yuan Huang]

So, perhaps we need a #ifdef MOZ_WIDGET_GONK around the pref in b2g.js?

Comment 48

[Nicolas B. Pierron [:nbp]]

(In reply to Ting-Yuan Huang from comment #47)
> So, perhaps we need a #ifdef MOZ_WIDGET_GONK around the pref in b2g.js?

You should use MOZ_B2G instead of MOZ_WIDGET_GONK, at least when it is not related to Gonk.  Otherwise we would have more differences between B2G desktop / Mulet and devices.

Comment 49

[Ting-Yuan Huang]

(In reply to Nicolas B. Pierron [:nbp] from comment #48)
> (In reply to Ting-Yuan Huang from comment #47)
> > So, perhaps we need a #ifdef MOZ_WIDGET_GONK around the pref in b2g.js?
> 
> You should use MOZ_B2G instead of MOZ_WIDGET_GONK, at least when it is not
> related to Gonk.  Otherwise we would have more differences between B2G
> desktop / Mulet and devices.

This is what I expect, because the unit test framework of b2g desktop uses toSource() :-( The other way would be changing the caller but I don't know if it is acceptable and how hard it would be.

Comment 50

[Bobby Holley (:bholley)]

(In reply to Ting-Yuan Huang from comment #45)
> Confirmed with Tim weeks ago and there should be no gaia codes using
> toSource(). Try server also said OK with my patch. However, I grep toSource
> in gaia and found two js files referring to it:

Note that toString() does the same thing as toSource() on a function, as does [[DefaultValue]] (i.e. someFunction + ""). So grepping for this could be hard.

> ./test_apps/test-agent/common/vendor/sinon/sinon.js:        if
> (source.toSource) {
> ./test_apps/test-agent/common/vendor/sinon/sinon.js:           
> target.toSource = function toSource() {
> ./test_apps/test-agent/common/vendor/sinon/sinon.js:                return
> source.toSource();
> ./test_apps/test-agent/common/vendor/sinon/sinon.js:            delete
> target.toSource;

This use of toSource here is strictly on Date objects, so I think we're ok. But toString() is another story. sinon does appear to use it on functions in a few places (though the big one, AFAICT, is just getting the function name, which should still work). If it doesn't break, it's probably ok.

As for r.js - we can/should disable source discarding for xpcshell. I'll write a patch to do that.

(In reply to Ting-Yuan Huang from comment #47)
> So, perhaps we need a #ifdef MOZ_WIDGET_GONK around the pref in b2g.js?

This (or anything of this sort) is a bad idea. We should avoid significant behavioral differences between desktop and gonk builds whenever possible.

Comment 51

[thomas tsai]

Hi Bobby: can you also generate a v1.3t patch?

Comment 52

[Bobby Holley (:bholley)]

Attachment: Part 7 - Flip off source discarding for XPCShell. v1

This is a just-in-case thing, to make it less risky.

Comment 53

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=4e935ae212dc

Comment 54

[Bobby Holley (:bholley)]

Attachment: source-discarding-tarako.tar.bz2

Here are the tarako patches, as best I can manage. I can't actually build the tarako branch locally [1], so these might not compile. But any fixups should be trivial.

Fabrice, let me know how it looks.

[1] https://pastebin.mozilla.org/4851919

Comment 55

[[:fabrice] Fabrice Desré]

I tested on tarako, I don't see any obvious major improvement (the video app was paused playing a video recorded with the camera app in both cases):

Without patches:

fabrice@fabrice-x240:~/dev/tarako$ b2g-info 
                            |     megabytes     |
           NAME   PID  PPID CPU(s) NICE  USS  PSS  RSS VSIZE OOM_ADJ USER     
            b2g 20915     1   29.3    0 27.7 30.5 38.7 140.6       0 root     
         (Nuwa) 20948 20915    0.9    0  0.3  1.7  6.8  49.4       0 root     
     Homescreen 20968 20948    3.7   18  6.4  9.6 19.0  63.5       8 app_20968
          Video 21069 20948    2.9    1  7.4 10.8 20.3  73.0       2 app_21069
(Preallocated a 21131 20948    1.1   18  5.5  7.6 14.8  58.4       1 root     

System memory info:

            Total 98.5 MB
     Used - cache 74.1 MB
  B2G procs (PSS) 60.2 MB
    Non-B2G procs 13.9 MB
     Free + cache 24.4 MB
             Free  2.0 MB
            Cache 22.4 MB

With bholley's patches

fabrice@fabrice-x240:~/dev/tarako$ b2g-info 
                            |     megabytes     |
           NAME   PID  PPID CPU(s) NICE  USS  PSS  RSS VSIZE OOM_ADJ USER     
            b2g 19979     1   30.0    0 27.5 30.2 38.2 135.8       0 root     
         (Nuwa) 20010 19979    0.9    0  0.5  2.0  7.5  49.4       0 root     
     Homescreen 20025 20010    3.8   18  7.2 10.3 19.5  64.6       8 app_20025
          Video 20094 20010    2.8    1  7.7 10.9 20.3  72.0       2 app_20094
(Preallocated a 20169 20010    1.1   18  5.9  8.0 15.3  56.4       1 root     

System memory info:

            Total 98.5 MB
     Used - cache 74.2 MB
  B2G procs (PSS) 61.3 MB
    Non-B2G procs 12.9 MB
     Free + cache 24.3 MB
             Free  1.7 MB
            Cache 22.6 MB

Comment 56

[[:fabrice] Fabrice Desré]

Attachment: tarako fixes

The fixes I applied to build on tarako. The only really annoying error was:
/home/fabrice/dev/mozilla-b2g28_v1_3t/js/xpconnect/src/XPCJSRuntime.cpp:84:   instantiated from here
../../../dist/include/mozilla/Atomics.h:837: error: static assertion failed: "mozilla/Atomics.h only supports 32-bit and pointer-sized types"

mochitest.ini has no test, only support files and that doesn't make the build system happy.

Comment 57

[Bobby Holley (:bholley)]

(In reply to Fabrice Desré [:fabrice] from comment #55)
> I tested on tarako, I don't see any obvious major improvement (the video app
> was paused playing a video recorded with the camera app in both cases):

Hm. I probably messed up the backport then, because comment 0 indicates that this should save ~4 megs of memory. Can someone who has a tarako and can build these patches locally take ownership of getting to the bottom of this?

Comment 58

[[:fabrice] Fabrice Desré]

Bobby, isn't that disabling the feature by default? Is there a pref to check also?
static mozilla::Atomic<bool> sDiscardSystemSource(false);

Comment 59

[Bobby Holley (:bholley)]

(In reply to Fabrice Desré [:fabrice] from comment #58)
> Bobby, isn't that disabling the feature by default? Is there a pref to check
> also?
> static mozilla::Atomic<bool> sDiscardSystemSource(false);

In part 6, we have

+ pref("javascript.options.discardSystemSource", true); 

in b2g/app/b2g.js 

In part 2, we have:

+ xpc::SetDiscardSystemSource(Preferences::GetBool(JS_OPTIONS_DOT_STR "discardSystemSource"));

in dom/base/nsJSEnvironment.cpp


Note that I had to totally rewrite the pref stuff in part 2 for tarako, because the code is different on 28. So it's quite possible that it's broken somehow. Putting it in nsJSContext::JSOptionChangedCallback was a bit of a hack.

Comment 60

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/487e6f72fcf7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0792729890cc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0fdd36b19a51
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/394e09fe2da2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/74b75c155205
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2926ad6c594f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6e1f9b821ae0

Comment 61

[Bobby Holley (:bholley)]

Followup bustage fix due to conflicts with bug 965970:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6b3c03454a4

Basically, the concept of NO_SOURCE moved from being a per-script thing to being a per-global thing, and XPConnect sets it on the global if we're System or in a privileged app. This makes more sense than unconditionally discarding source in the precompilation scope, since we want the eventual availability of source to match what would have happened had we not done precompilation at all.

Luke, is WMW a privileged app?

Comment 62

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7e51adede3b6 for gaia-integration bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=38138211&tree=Mozilla-Inbound

Comment 63

[Bobby Holley (:bholley)]

Try push with the gaia changes from bug 998799:

https://tbpl.mozilla.org/?tree=Try&rev=db5ae3b6dcf0

Comment 64

[Bobby Holley (:bholley)]

This is green. Once the PR in bug 998799 is merged, this can land.

Comment 65

[Bobby Holley (:bholley)]

fabrice and I just took a look at this on a Tarako. The savings are less there, because it turns out that we run a minifier during the build phase on Tarako, which we don't run elsewhere. So we don't get the full 4.3 MiB memory reduction that Ting-Yuan measured on a ZTE Open in comment 21.

The overall savings of this patch on Tarako are something like 1.26 MiB. The actual reduction in script-sources was around 2.4 MiB, but there were other regressions (> 1MiB) that cut into that.

The regressions were split across script-data, unused-gc-things, and heap waste. This warrants some investigation, which I'll do in a separate bug.

Comment 66

[Ting-Yuan Huang]

The 4.3MB came from b2g + homescreen and IIUC already counted the regression of script-data in, i.e., it's ~5MB saving of script-sources. I'm wondering if the regression in heaps are indeed fragmentation or freed-but-dirty pages by BytecodeCompiler and should be fine in all practical applications.

Comment 67

[Joe Cheng [:jcheng] (please needinfo)]

hi Bobby, do you feel this is safe enough to land in 1.3T? we'd like to close off more of the tarako work at the end of April. if it's too risky, we should reconsider. Thanks

Comment 68

[Luke Wagner [:luke]]

(In reply to Bobby Holley (:bholley) from comment #61)
> Followup bustage fix due to conflicts with bug 965970:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f6b3c03454a4

Wait, does this mean that pre-compilation is now retaining (i.e., memcpy'ing) the source?  This seems pretty wasteful in time and increasing likelihood of OOM.  Given that we've just created a new global, shouldn't we be able to flag it as discard-source?

> Luke, is WMW a privileged app?

Nope, and the general target for precompilation is non-privileged apps.

Comment 69

[Bobby Holley (:bholley)]

(In reply to Joe Cheng [:jcheng] from comment #67)
> hi Bobby, do you feel this is safe enough to land in 1.3T? we'd like to
> close off more of the tarako work at the end of April. if it's too risky, we
> should reconsider. Thanks

Yes, I think we should uplift it. Fabrice has the backported patches I wrote and is going to do it.

Comment 70

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #68)
> (In reply to Bobby Holley (:bholley) from comment #61)
> > Followup bustage fix due to conflicts with bug 965970:
> > 
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/f6b3c03454a4
> 
> Wait, does this mean that pre-compilation is now retaining (i.e.,
> memcpy'ing) the source?  This seems pretty wasteful in time and increasing
> likelihood of OOM.  Given that we've just created a new global, shouldn't we
> be able to flag it as discard-source?

Maybe I misunderstood. My assumption was that, when we compile the script "for real", we'd use the version from the compilation global in the event of a cache hit, so the decision about whether or not to discard source in the precompilation global would propagate to the end-consumer (and WMW would suddenly have [sourceless] functions). Is this the case, or is it purely a bytecode cache? If the latter, then it seems like we should definitely discard source for the precompilation global, and I can file a bug and do that.

Comment 71

[Luke Wagner [:luke]]

(In reply to Bobby Holley (:bholley) from comment #70)
When we compile "for real", we get a cache hit and deserialize the compiled asm.js module (not bytecode, but machine code plus metadata), reusing the ScriptSource of the current compilation (that has already been created for the script containing the asm.js module).  Thus, the sourcePolicy at asm.js-compilation time is irrelevant and pure overhead.

Comment 72

[Bobby Holley (:bholley)]

Attachment: Part 8 - Make discardSource a Sandbox option and use it (even in Desktop) for the precompilation scope. v1

Comment 73

[Luke Wagner [:luke]]

Comment on attachment 8410417 [details] [diff] [review]
Part 8 - Make discardSource a Sandbox option and use it (even in Desktop) for the precompilation scope. v1

Awesome, thanks!

Comment 74

[[:fabrice] Fabrice Desré]

remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/304e63b3eeab
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/bdd5aef9d2e6
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/2d4e014448a9
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/3a9984542b03
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/11f89146ebe1
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/040ce4f8dcb0
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/46e50e7485c2
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/733178725198

Comment 75

[Bobby Holley (:bholley)]

The failures from comment 62 should be fixed by bug 998799, which has now been picked up on m-c and m-i. Per comment 63, this should be green on Gi now.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/95391b25bc11
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/26a22df461a2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/42aaedb93455
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6f6abed86df7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c630ff2cdf9c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/744e3fa4bb8a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8ff30e42330e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a256a26923ff

Comment 76

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/95391b25bc11
https://hg.mozilla.org/mozilla-central/rev/26a22df461a2
https://hg.mozilla.org/mozilla-central/rev/42aaedb93455
https://hg.mozilla.org/mozilla-central/rev/6f6abed86df7
https://hg.mozilla.org/mozilla-central/rev/c630ff2cdf9c
https://hg.mozilla.org/mozilla-central/rev/744e3fa4bb8a
https://hg.mozilla.org/mozilla-central/rev/8ff30e42330e
https://hg.mozilla.org/mozilla-central/rev/a256a26923ff

Comment 77

[Ting-Yuan Huang]

I'm still seeing non-trivial script-sources (2.9MB in b2g and hundred KBs in others) in today's build of 1.3t on tarako in memory reports. It should be enabled by default, right? Or, is this just a false alarm that we forgot to remove the entries from memory reports? I didn't see noticeable script-sources in a m-c build on unagi.

Comment 78

[Bobby Holley (:bholley)]

(In reply to Ting-Yuan Huang from comment #77)
> I'm still seeing non-trivial script-sources (2.9MB in b2g and hundred KBs in
> others) in today's build of 1.3t on tarako in memory reports. It should be
> enabled by default, right?

Yes.

> Or, is this just a false alarm that we forgot to
> remove the entries from memory reports?

No, the memory reports are generated dynamically.

Given that you can reproduce the bug, can you please investigate this Ting-Yuan?

Comment 79

[[:fabrice] Fabrice Desré]

What happens is that when the BackstagePass global is created, ShouldDiscardSystemSource() returns false, because SetDiscardSystemSource() has not been called yet.

Comment 80

[[:fabrice] Fabrice Desré]

Attachment: script-source.patch

Terrible hack that works, but it's likely that there is a better place to call xpc::SetDiscardSystemSource(Preferences::GetBool(JS_OPTIONS_DOT_STR "discardSystemSource")) early on.

Comment 81

[[:fabrice] Fabrice Desré]

We discussed the right fix with bholley, this get us to 10MB of js-non-window instead of 13MB in the parent.

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/940ccf82acd0

Comment 82

[Diego Marcos [:dmarcos]]

Fabrice, is there a quick way to get debugging back for certified apps? If not we should back this patch out. I have a very hard time to do my job if I cannot use the debugger.

Comment 83

[Bobby Holley (:bholley)]

(In reply to Diego Marcos [:dmarcos] from comment #82)
> Fabrice, is there a quick way to get debugging back for certified apps? If
> not we should back this patch out. I have a very hard time to do my job if I
> cannot use the debugger.

See bug 1001348 comment 30. We should talk about it there.

Comment 84

[Ting-Yuan Huang]

Fabrice's fix in comment 81 looks great. Verified on a real Tarako.

Comment 85

[Kai-Zhen Li [:seinlin][:kli]]

Got feedback from partner that app-manager can't view javascript source.

Is it possible to have an option such as "ac_add_options --enable-script-sources" in gonk-misc/default-gecko-config which allo developer to veiw source in app-manager?

By default it is disabled.

Comment 86

[Bobby Holley (:bholley)]

(In reply to Kai-Zhen Li from comment #85)
> Got feedback from partner that app-manager can't view javascript source.
> 
> Is it possible to have an option such as "ac_add_options
> --enable-script-sources" in gonk-misc/default-gecko-config which allo
> developer to veiw source in app-manager?
> 
> By default it is disabled.

It's even easier. You just need to flip the javascript.options.discardSystemSource pref to |false|, either in the profile directory, or at compile-time in b2g/app/b2g.js.

Comment 87

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Kai-Zhen Li from comment #85)
> Got feedback from partner that app-manager can't view javascript source.
> 
> Is it possible to have an option such as "ac_add_options
> --enable-script-sources" in gonk-misc/default-gecko-config which allo
> developer to veiw source in app-manager?
> 
> By default it is disabled.

We're attempting to solve the App Manager use case in bug 1001348.  Let's keep related discussion in that bug.  There's a fix attached that will flip the pref Bobby mentioned when using DEVICE_DEBUG with Gaia.

Comment 88

[chaofei.wu]

remake[about_RAM_performance]

Comment 89

[Eric Rahm [:erahm]]

So it looks like the patch to make all of this work noted in comment 81 never landed on m-c. Was that intentional or did we not need it on m-c?