Pre-compile JSM scripts into the shared JSM global rather than compilation scope

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(2 attachments)

Assignee

Description

2 years ago
We currently off-thread pre-compile JSM scripts into xpc::CompilationScope, and then clone them into the module's global scope before executing. This is still much faster than not pre-compiling, but in a startup profile on my developer machine, we still spend about 43ms before first paint cloning.

If we compile into the shared JSM global instead, none of the JSM scripts need to be cloned, and we get all of that overhead back.
Assignee

Comment 1

2 years ago
Although, it's probably worth noting that we'll still need to clone the script to execute in a non-syntactic rather than global scope (since off-thread decoding only supports the latter), and that may not be any cheaper.
Comment hidden (mozreview-request)
Assignee

Comment 4

2 years ago
I haven't done any talos runs for this yet, but when I test it locally, it gives a 30-50ms improvement on top of bug 1186409.
Whiteboard: [qf]

Comment 5

2 years ago
mozreview-review
Comment on attachment 8901326 [details]
Bug 1381976: Part 1 - Use the shared module global for script pre-compilation.

https://reviewboard.mozilla.org/r/172788/#review178220

::: js/xpconnect/loader/ScriptPreloader.cpp:1009
(Diff revision 1)
> -    AutoJSAPI jsapi;
> +    AutoSafeJSAPI jsapi;
> -    MOZ_RELEASE_ASSERT(jsapi.Init(xpc::CompilationScope()));
>      JSContext* cx = jsapi.cx();
>  
> +    JSAutoCompartment ac(cx, CompilationScope(cx));
> +    JS::Rooted<JS::ScriptVector> jsScripts(cx, JS::ScriptVector(cx));

jsScripts doesn't look like it is used.

::: js/xpconnect/loader/mozJSComponentLoader.h:86
(Diff revision 1)
>                              JS::MutableHandleObject aGlobal);
>  
>      bool ReuseGlobal(bool aIsAddon, nsIURI* aComponent);
>  
> +    friend class ScriptPreloader;
> +    JSObject* CompilationScope(JSContext* aCx)

It is odd for CompilationScope() to return null when global sharing is disabled. You should rename it to something like SharedLoaderGlobal or make it return the default XPC compilation scope instead of null.

::: js/xpconnect/loader/mozJSComponentLoader.h:92
(Diff revision 1)
> +    {
> +        if (mLoaderGlobal)
> +            return mLoaderGlobal;
> +        if ((mInitialized || NS_SUCCEEDED(ReallyInit())) &&
> +            mShareLoaderGlobal)
> +            return GetSharedGlobal(aCx);

The condition is on multiple lines, so you should brace the body of the if.

::: js/xpconnect/loader/mozJSComponentLoader.cpp:577
(Diff revision 1)
> +
> +        // AutoEntryScript required to invoke debugger hook, which is a
> +        // Gecko-specific concept at present.
> +        dom::AutoEntryScript aes(globalObj,
> +                                 "component loader report global");
> +        JS_FireOnNewGlobalObject(aes.cx(), globalObj);

Doesn't this mean you are calling this twice on the new global? You didn't remove any calls to it. Also, is it okay to call this before the global is set up with __URI__ and so forth?
Attachment #8901326 - Flags: review?(continuation) → review-

Comment 6

2 years ago
mozreview-review
Comment on attachment 8901327 [details]
Bug 1381976: Part 2 - Cleanup private/protected members and mark class final.

https://reviewboard.mozilla.org/r/172790/#review178230
Attachment #8901327 - Flags: review?(continuation) → review+
Assignee

Comment 7

2 years ago
mozreview-review-reply
Comment on attachment 8901326 [details]
Bug 1381976: Part 1 - Use the shared module global for script pre-compilation.

https://reviewboard.mozilla.org/r/172788/#review178220

> jsScripts doesn't look like it is used.

Urgh. I copied one more line than I meant to.

> It is odd for CompilationScope() to return null when global sharing is disabled. You should rename it to something like SharedLoaderGlobal or make it return the default XPC compilation scope instead of null.

Heh. I initially did have it return the compilation scope when shared globals were disabled, but decided to change it. I'm not really sure why.

> Doesn't this mean you are calling this twice on the new global? You didn't remove any calls to it. Also, is it okay to call this before the global is set up with __URI__ and so forth?

Nope, because now we don't set `createdNewGlobal` to when `reuseGlobal` is set.

And, yes, it should be fine to call it before we set `__URI__` and so forth. Those don't really have any meaning for the shared module global (and don't get set on the global object for it anyway), and the debugger doesn't actually care about those properties, anyway. They're just there for the script being loaded into it.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Kris Maglione [:kmag] from comment #7)
> Nope, because now we don't set `createdNewGlobal` to when `reuseGlobal` is
> set.

Ah, right.

> And, yes, it should be fine to call it before we set `__URI__` and so forth.
> Those don't really have any meaning for the shared module global (and don't
> get set on the global object for it anyway), and the debugger doesn't
> actually care about those properties, anyway. They're just there for the
> script being loaded into it.

Good to know. It might make sense to get rid of createdNewGlobal entirely then, and just move the debugging hook stuff right next to when we create the global in PrepareObjectForLocation().

Comment 11

2 years ago
mozreview-review
Comment on attachment 8901326 [details]
Bug 1381976: Part 1 - Use the shared module global for script pre-compilation.

https://reviewboard.mozilla.org/r/172788/#review178278
Attachment #8901326 - Flags: review?(continuation) → review+
Assignee

Comment 12

2 years ago
(In reply to Andrew McCreight [:mccr8] from comment #10)
> Good to know. It might make sense to get rid of createdNewGlobal entirely
> then, and just move the debugging hook stuff right next to when we create
> the global in PrepareObjectForLocation().

Yeah, maybe. I was trying to minimize unnecessary changes, but I'll file a separate bug for this.
I just noticed this comment:
    // Defer firing OnNewGlobalObject until after the __URI__ property has
    // been defined so the JS debugger can tell what module the global is
    // for

So maybe this shouldn't be changed in this way? Though it is just a bit weird in the shared JSM world, because set the __URI__ property on the NSVO, not the global.
Bug 986757 is where that comment was added. Maybe that will explain what is needed.
Assignee

Comment 15

2 years ago
(In reply to Andrew McCreight [:mccr8] from comment #13)
> I just noticed this comment:
>     // Defer firing OnNewGlobalObject until after the __URI__ property has
>     // been defined so the JS debugger can tell what module the global is
>     // for
> 
> So maybe this shouldn't be changed in this way? Though it is just a bit
> weird in the shared JSM world, because set the __URI__ property on the NSVO,
> not the global.

That doesn't apply to the shared module global, since we don't define a __URI__ on it anyway, and if we did, it wouldn't apply to any particular module.

Bug 986757 is about the add-on debugger, which we don't have to worry about post-57 (WebExtensions don't use JSMs, so this doesn't apply to them). But even if we did, we still load add-on modules into their own globals, so the hook still fires after __URI__ is defined for them.
Ah, right, that makes sense...
Whiteboard: [qf] → [qf:p1]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

2 years ago
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d7cef820d5fe
Part 1 - Use the shared module global for script pre-compilation. r=mccr8
https://hg.mozilla.org/integration/autoland/rev/6e30ea616788
Part 2 - Cleanup private/protected members and mark class final. r=mccr8
https://hg.mozilla.org/mozilla-central/rev/d7cef820d5fe
https://hg.mozilla.org/mozilla-central/rev/6e30ea616788
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Updated

2 years ago
Depends on: 1404741
You need to log in before you can comment on or make changes to this bug.