Closed Bug 990353 Opened 6 years ago Closed 5 years ago

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

Categories

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

All
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla31
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3T --- fixed
b2g-v1.4 --- ?
b2g-v2.0 --- ?

People

(Reporter: laszio.bugzilla, Assigned: bholley)

References

(Blocks 1 open bug)

Details

(Keywords: memory-footprint, perf, Whiteboard: [c=memory p= s=2014.04.25 u=tarako] [MemShrink:P1])

Attachments

(11 files, 8 obsolete files)

849.46 KB, text/plain
Details
27.58 KB, patch
luke
: review+
Details | Diff | Splinter Review
5.07 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
1.73 KB, patch
mshal
: review+
Details | Diff | Splinter Review
5.50 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
1.02 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
5.62 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
1.05 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
10.38 KB, application/x-bzip2
fabrice
: feedback+
Details
2.65 KB, patch
Details | Diff | Splinter Review
6.34 KB, patch
luke
: review+
Details | Diff | Splinter Review
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.
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.
Attachment #8399776 - Flags: review?(gkrizsanits)
Whiteboard: [MemShrink]
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 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 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.
Attachment #8399776 - Flags: review?(gkrizsanits) → review?(bobbyholley)
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.
Attachment #8399776 - Flags: review?(bobbyholley) → review-
(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?
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.
Flags: needinfo?(timdream)
blocking-b2g: --- → 1.3T?
Summary: Don't save script-souces for B2G chrome and built-in apps → Don't save script-sources for B2G chrome and built-in apps
Whiteboard: [MemShrink] → [MemShrink:P1]
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 ....)
Flags: needinfo?(timdream)
Attached patch WIP -- nosource.diff (obsolete) — Splinter Review
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?
Attachment #8401185 - Flags: review?(bobbyholley)
Attachment #8401185 - Flags: feedback?(nicolas.b.pierron)
Attachment #8399776 - Attachment is obsolete: true
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.
Attachment #8401185 - Flags: review?(bobbyholley) → review-
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.
triage: not blocking tarako release but when we have r+, let's reconsider for uplift to 1.3T thanks
blocking-b2g: 1.3T? → -
Attached patch WIP -- nosource.diff v.2 (obsolete) — Splinter Review
(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.
Attachment #8401185 - Attachment is obsolete: true
Attachment #8401185 - Flags: feedback?(nicolas.b.pierron)
Attachment #8402595 - Flags: feedback?(bobbyholley)
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.
Attachment #8402595 - Flags: feedback?(bobbyholley) → feedback+
(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?
(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. :-(
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.
Attachment #8402595 - Attachment is obsolete: true
Attachment #8403768 - Flags: review?(bobbyholley)
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.
Assignee: nobody → bobbyholley
Attachment #8403768 - Attachment is obsolete: true
Attachment #8403768 - Flags: review?(bobbyholley)
Attached patch rollup patch (obsolete) — Splinter Review
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.
Attachment #8404973 - Flags: feedback?(thuang)
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)
Attachment #8404973 - Attachment description: discarding.patch → rollup patch
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.
Attachment #8404973 - Flags: feedback?(thuang)
Firefox OS::General is rarely the right place for a bug.
Component: General → DOM
Product: Firefox OS → Core
Status: NEW → ASSIGNED
Keywords: footprint, perf
Priority: -- → P1
Whiteboard: [MemShrink:P1] → [c=memory p= s= u=] [MemShrink:P1]
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.
Flags: needinfo?(bent.mozilla)
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.
Flags: needinfo?(bent.mozilla)
Attached patch discarding-rollup-v2.patch (obsolete) — Splinter Review
Ting-Yuan: this patch should fix things for the RIL worker. Can you have a look?
Attachment #8404973 - Attachment is obsolete: true
Attachment #8405688 - Flags: feedback?(thuang)
Blocks: 995537
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.
Attachment #8405705 - Flags: review?(luke)
Attachment #8405706 - Flags: review?(gkrizsanits)
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.
Attachment #8405706 - Flags: review?(gkrizsanits) → review?(bent.mozilla)
We also add a mochitest.ini file, which will eventually be used for http-served
files that are associated with chrome mochitests.
Attachment #8405708 - Flags: review?(gps)
Attachment #8405709 - Flags: review?(bent.mozilla)
Attachment #8405710 - Flags: review?(bent.mozilla)
Comment on attachment 8405688 [details] [diff] [review]
discarding-rollup-v2.patch

Confirmed here. The result is as good as expected.
Attachment #8405688 - Flags: feedback?(thuang) → feedback+
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?
Attachment #8405705 - Flags: review?(luke) → review+
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.
Attachment #8405706 - Flags: review?(bent.mozilla) → review-
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'
Attachment #8405707 - Flags: review?(bent.mozilla) → review+
Comment on attachment 8405709 [details] [diff] [review]
Part 5 - Tests. v1

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

\o/
Attachment #8405709 - Flags: review?(bent.mozilla) → review+
Attachment #8405710 - Flags: review?(bent.mozilla) → review+
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...
(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.
Flags: needinfo?(thuang)
(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.
Attachment #8405688 - Attachment is obsolete: true
Attachment #8405706 - Attachment is obsolete: true
Attachment #8407874 - Flags: review?(bent.mozilla)
Attachment #8407874 - Flags: review?(bent.mozilla) → review+
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!
Attachment #8405708 - Flags: review?(gps) → review+
OK to take this to 1.3T?
blocking-b2g: - → 1.3T?
(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?
Flags: needinfo?(thuang) → needinfo?(timdream)
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.
Flags: needinfo?(timdream)
So, perhaps we need a #ifdef MOZ_WIDGET_GONK around the pref in b2g.js?
(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.
(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.
(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.
Hi Bobby: can you also generate a v1.3t patch?
This is a just-in-case thing, to make it less risky.
Attachment #8409145 - Flags: review?(fabrice)
Attachment #8409145 - Flags: review?(fabrice) → review+
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
Attachment #8409166 - Flags: feedback?(fabrice)
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
Attached patch tarako fixesSplinter Review
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.
Attachment #8409166 - Flags: feedback?(fabrice) → feedback+
(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?
Bobby, isn't that disabling the feature by default? Is there a pref to check also?
static mozilla::Atomic<bool> sDiscardSystemSource(false);
(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.
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?
Flags: needinfo?(luke)
Depends on: 998799
Try push with the gaia changes from bug 998799:

https://tbpl.mozilla.org/?tree=Try&rev=db5ae3b6dcf0
This is green. Once the PR in bug 998799 is merged, this can land.
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.
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.
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
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(luke)
(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.
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(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.
Flags: needinfo?(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!
Attachment #8410417 - Flags: review?(luke) → review+
Whiteboard: [c=memory p= s= u=] [MemShrink:P1] → [c=memory p= s=2014.04.25 u=tarako] [MemShrink:P1]
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.
Flags: needinfo?(fabrice)
Flags: needinfo?(bobbyholley)
(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?
Flags: needinfo?(thuang)
Flags: needinfo?(fabrice)
Flags: needinfo?(bobbyholley)
What happens is that when the BackstagePass global is created, ShouldDiscardSystemSource() returns false, because SetDiscardSystemSource() has not been called yet.
Attached patch script-source.patch (obsolete) — Splinter Review
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.
Attachment #8417722 - Flags: review?(bobbyholley)
Attachment #8417722 - Attachment is obsolete: true
Attachment #8417722 - Flags: review?(bobbyholley)
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
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.
Flags: needinfo?(fabrice)
(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.
Flags: needinfo?(fabrice)
Fabrice's fix in comment 81 looks great. Verified on a real Tarako.
Flags: needinfo?(thuang)
Depends on: 1006695
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.
Flags: needinfo?(thuang)
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(thuang)
Depends on: 1020948
Depends on: 1049876
remake[about_RAM_performance]
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?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.