1356810 - Compile subscript loader scripts with noScriptRval by default

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Kris Maglione [:kmag]]

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.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1356810: Use noScriptRval option by default for loadSubScript.

Review commit: https://reviewboard.mozilla.org/r/130500/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130500/

Comment 2

[Kris Maglione [:kmag]]

(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.

Comment 3

[Andrew McCreight [:mccr8]]

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

Comment 4

[Kris Maglione [:kmag]]

(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 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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 6

[Kris Maglione [:kmag]]

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 :)

Comment 7

[Kris Maglione [:kmag]]

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

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (: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?

Comment 9

[Kris Maglione [:kmag]]

(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.

Comment 10

[Kris Maglione [:kmag]]

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

Comment 11

[Kris Maglione [:kmag]]

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/20dff607fb88
Follow-up: Fix SDK worker tests. a=merge

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bf8468e259dc

Comment 15

[Gary [:streetwolf52]]

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.

Comment 16

[Gary [:streetwolf52]]

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

Comment 17

[Kris Maglione [:kmag]]

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

Comment 18

[Gerry Chang [:gchang]]

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