Closed Bug 1381976 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: XPConnect, enhancement)

Product:
Core
Component:
XPConnect
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1381976: Part 1 - Use the shared module global for script pre-compilation.
59 bytes, text/x-review-board-request
mccr8
: review+
Details
Bug 1381976: Part 2 - Cleanup private/protected members and mark class final.
59 bytes, text/x-review-board-request
mccr8
: review+
Details 
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.
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.
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 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 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+
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.
(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 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+
(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.
(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]
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
Depends on: 1404741
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: