Add lazy version of importGlobalProperties

RESOLVED FIXED in Firefox 62

Status

()

enhancement
RESOLVED FIXED
Last year
9 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks 1 bug)

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(5 attachments)

Assignee

Description

Last year
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 hidden (mozreview-request)

Comment 5

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

https://reviewboard.mozilla.org/r/247024/#review253768
Attachment #8980835 - Flags: review?(continuation) → review+

Comment 6

Last year
mozreview-review
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?
Attachment #8980836 - Flags: review?(continuation) → review+

Comment 7

Last year
mozreview-review
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?
Attachment #8980838 - Flags: review?(continuation) → review+
Assignee

Comment 8

Last year
mozreview-review-reply
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

Last year
mozreview-review
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.
Attachment #8980837 - Flags: review?(standard8) → review+
Assignee

Comment 10

Last year
(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.
Assignee

Comment 11

Last year
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
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.
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)
You need to log in before you can comment on or make changes to this bug.