Compiling scripts to return a value disables optimizations and prevents JITting the top-level scope. Compiling with noScriptRval on my machine saves 10ms (or ~20%) on just loading the devtools startup code, and 20ms (16%) on overall startup.

(In reply to Kris Maglione [:kmag] from comment #0) > Compiling scripts to return a value disables optimizations and prevents > JITting the top-level scope. Compiling with noScriptRval on my machine saves > 10ms (or ~20%) on just loading the devtools startup code, and 20ms (16%) on > overall startup. Er, rather, and 20ms on add-on manager startup. 30ms total from the two combined.

I have some code cleanups to this file in progress in bug 1356666 and bug 1356799 you may have to rebase around.

(In reply to Andrew McCreight [:mccr8] from comment #3) > I have some code cleanups to this file in progress in bug 1356666 and bug > 1356799 you may have to rebase around. No complaints from me. That file has been progressively turning into a nightmare since b2g started using it...

Comment on attachment 8858531 [details] Bug 1356810: Use noScriptRval option by default for loadSubScript. https://reviewboard.mozilla.org/r/130500/#review134000 ::: js/xpconnect/idl/mozIJSSubScriptLoader.idl:45 (Diff revision 1) > * - target: an object to evaluate onto (default: global object of the caller) > * - ignoreCache: if set to true, will bypass the cache for reading the file. > * - async: if set to true, the script will be loaded > * asynchronously, and a Promise is returned which > * resolves to its result when execution is complete. > + * - wantReturnValue: If true, the script will return Would be nice to say that this defaults to true.

Comment on attachment 8858531 [details] Bug 1356810: Use noScriptRval option by default for loadSubScript. https://reviewboard.mozilla.org/r/130500/#review134000 > Would be nice to say that this defaults to true. Well, it defaults to false :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c706be68a0742078cb76c03f58a8e9d01de2dec Bug 1356810: Use noScriptRval option by default for loadSubScript. r=billm

(In reply to Kris Maglione [:kmag] from comment #6) > Comment on attachment 8858531 [details] > Bug 1356810: Use noScriptRval option by default for loadSubScript. > > https://reviewboard.mozilla.org/r/130500/#review134000 > > > Would be nice to say that this defaults to true. > > Well, it defaults to false :) Oops, I misread the code for ParseBoolean. Aren't we at risk of breaking some add-ons this way? Should we at least flag this as addon-compat?

(In reply to Bill McCloskey (:billm) from comment #8) > Oops, I misread the code for ParseBoolean. Aren't we at risk of breaking > some add-ons this way? Should we at least flag this as addon-compat? I searched DXR, and I couldn't find any add-ons that actually appeared to use the return value. The closest I could find was a bit of the SDK sandbox code that returns the value of loadSubScript when it's using that rather than evalInSandbox, but I don't think that's being used anywhere either. I suppose there's an outside chance that someone is using it, though, so it can't hurt to add the flag.

https://hg.mozilla.org/integration/mozilla-inbound/rev/93adb36bc1d2b22fb5357e6e5714626db3770e42 Bug 1356810: Follow-up: Fix xpcshell test for async subscript return value.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf8468e259dce509bfe0c4cc51874a0797932e36 Bug 1356810: Follow-up: Fix SDK worker tests.

Pushed by kwierso@gmail.com: https://hg.mozilla.org/mozilla-central/rev/20dff607fb88 Follow-up: Fix SDK worker tests. a=merge

https://hg.mozilla.org/mozilla-central/rev/5c706be68a07 https://hg.mozilla.org/mozilla-central/rev/93adb36bc1d2 https://hg.mozilla.org/mozilla-central/rev/20dff607fb88

Just wondering if this patch has broken the add-on LastPass? It's in the regression range I determined. LP no longer fills in forms automatically. LP has already been broken by another patch https://bugzilla.mozilla.org/show_bug.cgi?id=1356883 although auto fill was still working until this newer patch.

I appears that whatever was causing the LP auto fill problem is no longer present on inbound.

Comment on attachment 8858531 [details] Bug 1356810: Use noScriptRval option by default for loadSubScript. Approval Request Comment [Feature/Bug causing the regression]: N/A [User impact if declined]: This change significantly improves startup performance, and should help mitigate the startup performance regressions from enabling Screenshots on beta [Is this code covered by automated tests?]: To some extent. Its consumers are covered by more tests than the interface itself. [Has the fix been verified in Nightly?]: N/A [Needs manual test from QE? If yes, steps to reproduce]: N/A [List of other uplifts needed for the feature/fix]: None, but it will need some rebasing due to recent refactoring in the subscript loader. Uplifting bug 1356666 and bug 1356799 would help. [Is the change risky?]: Low-risk [Why is the change risky/not risky?]: This simply prevents loadSubScript from returning the result value of script evaluation. That feature was never widely-known or widely-used. The only real risk is that this may affect some add-ons, but it hasn't caused add-on issues so far, and dxr suggests that it's not likely to. [String changes made/needed]: None

Screenshots will go live in Fx55 so we don't have to uplift these patches in 54. Beta54-. Mark 54 won't fix.