1464548 - Add lazy version of importGlobalProperties

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement)

Description

[Kris Maglione [:kmag]]

It looks like we currently create fetch and XMLHttpRequest bindings in multiple globals in each content process, even though they're generally not used in content processes. Those prototype objects are fairly expensive to create, so we should create them lazily in cases where we're not likely to need them.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1464548: Part 1a - Add defineLazyGlobalGetters helper.

This allows us to lazily import global properties using
Cu.importGlobalProperties. Aside from making it easier to avoid lazily
importing these properties, it also defines them all in the shared JSM global
so that we don't risk re-creating them in Sandboxes or frameloader globals.

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

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1464548: Part 2b - Don't delete properties before redefining them, because deleting properties kills JIT performance.

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

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1464548: Part 2 - Add ESLint support for defineLazyGlobalGetters.

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

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1464548: Part 3 - Update callers to use defineLazyGlobalGetters.

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

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 8980835 [details]
Bug 1464548: Part 1a - Add defineLazyGlobalGetters helper.

https://reviewboard.mozilla.org/r/247024/#review253768

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8980836 [details]
Bug 1464548: Part 2b - Don't delete properties before redefining them, because deleting properties kills JIT performance.

https://reviewboard.mozilla.org/r/247026/#review253784

I'm not too familiar with this, but it looks okay to me.

::: js/xpconnect/loader/XPCOMUtils.jsm:193
(Diff revision 1)
>     *        A function that returns what the getter should return.  This will
>     *        only ever be called once.
>     */
>    defineLazyGetter: function XPCU_defineLazyGetter(aObject, aName, aLambda)
>    {
> +    let redefining = false;

I guess this could create an additional closure but hopefully that doesn't matter.

::: js/xpconnect/loader/XPCOMUtils.jsm:235
(Diff revision 1)
> -          }
>            Services.scriptloader.loadSubScript(aResource, aObject);
>            return aObject[name];
>          },
> +        set(value) {
> +          redefine(aObject, name, value);

So is the idea here that loadSubScript will hit this new setter and thus replace the property without triggering the proxy?

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 8980838 [details]
Bug 1464548: Part 3 - Update callers to use defineLazyGlobalGetters.

https://reviewboard.mozilla.org/r/247030/#review253812

You should post about this in firefox-dev. It seems like something handy that anybody interested in load time would like to know about. I know at least Felipe is looking at content process load time.

Did you just change everything, or only places where you checked that the property wasn't obviously used at the top level, or what?

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8980838 [details]
Bug 1464548: Part 3 - Update callers to use defineLazyGlobalGetters.

https://reviewboard.mozilla.org/r/247030/#review253812

I changed pretty much everything outside of tests, double-checked to make sure everything was sane, and then added some logging to make sure we didn't trigger any of the getters in the content process in a normal run.

I think I might have left a couple that seemed OK without lazy getters.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8980837 [details]
Bug 1464548: Part 2 - Add ESLint support for defineLazyGlobalGetters.

https://reviewboard.mozilla.org/r/247028/#review253968

Thanks, r=Standard. 

Just to note, it has been my suspicion for a while that at least some of the current importGlobalProperties are unnecessary. Proving that is hard outside the jsm scope though (xref bug 1415483). At least this bug will help remove any harm they are doing.

Comment 10

[Kris Maglione [:kmag]]

(In reply to Mark Banner (:standard8) from comment #9)
> Just to note, it has been my suspicion for a while that at least some of the
> current importGlobalProperties are unnecessary. Proving that is hard outside
> the jsm scope though (xref bug 1415483). At least this bug will help remove
> any harm they are doing.

I actually found a bunch of unnecessary uses when I added this rule, since it turned the unused imports into errors.

Comment 11

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f738942056e19cbe90efdd8a455e149008ef2deb
Bug 1464548: Part 1a - Add defineLazyGlobalGetters helper. r=mccr8

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a8cd93316d6785a593b204bc204d6004449d76a
Bug 1464548: Part 1b - Don't delete properties before redefining them, because deleting properties kills JIT performance. r=mccr8

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6e7bfa38103efc76635dfd8d4ecd6bf51a6c590
Bug 1464548: Part 2 - Add ESLint support for defineLazyGlobalGetters. r=standard8

https://hg.mozilla.org/integration/mozilla-inbound/rev/22daf991f7b705c6849c5597f9794f1ac8f86a8e
Bug 1464548: Part 3 - Update callers to use defineLazyGlobalGetters. r=mccr8

Comment 12

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b08fd277ce5f750fbec6aaa239f3c7c9806e22a
Bug 1464548: Fix linux32 xpcshell bustage. r=bustage

Comment 13

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/f738942056e1
https://hg.mozilla.org/mozilla-central/rev/4a8cd93316d6
https://hg.mozilla.org/mozilla-central/rev/a6e7bfa38103
https://hg.mozilla.org/mozilla-central/rev/22daf991f7b7
https://hg.mozilla.org/mozilla-central/rev/5b08fd277ce5

Comment 14

[[github robot]]

Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/0cf21c41e96d81cfa143b7da96a9959f8e4c4abf
chore(mc): Port Bug 1464548: Part 3 - Update callers to use defineLazyGlobalGetters. r=mccr8
Switched to warning eslint no-undef because https://hg.mozilla.org/mozilla-central/rev/a6e7bfa38103 will be available in something newer than 0.13.1.

Comment 15

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4205

Comment 16

[[github robot]]

Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/6c48ecd1de0fd854be25eb11b57489f2c13f9972
chore(lint): Followup Bug 1464548 - Restore eslint no-undef by upgrading to eslint-plugin-mozilla 0.14.0 (#4205)