1364816 - Object.getOwnPropertyNames(window) is slow

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Markus Stange [:mstange]]

On my machine, this testcase prints around 16ms in Firefox and 0.2ms in Chrome.

Profile: https://perfht.ml/2qhHW9e

We seem to be creating all bindings that can be accessed on the window object, even though we only need the names of the window properties, not their values.

Comment 1

[Markus Stange [:mstange]]

Attachment: testcase

Comment 2

[Markus Stange [:mstange]]

This bug is easy to hit with a common JavaScript toolchain: The "transform-runtime" plugin for the babel compiler transitively includes the following module, which calls Object.getOwnPropertyNames(window) during initialization:
https://github.com/zloirock/core-js/blob/master/library/modules/_object-gopn-ext.js

I've created a simple repository that shows the babel settings and an example JS file which are sufficient to encounter this bug: https://github.com/mstange/testcase-babel-windowbinding

This is also affecting perf.html.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

That is only the first access to those properties, right?
Do I recall correctly that Blink/V8 pay the penalty earlier, when creating the Window object.

Comment 4

[Markus Stange [:mstange]]

Yes, only on the first access.
I don't know when Blink pays this penalty, but it would be nice to find out how much of a penalty it is for them.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

bz or peterv would know better.

IIRC at least at some point creating a new Window object was slower in Blink than in Gecko.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Our window creation is way faster than Blink's precisely because we do the work lazily.

That said, I'd be interested in seeing how the second call performs relative to the first.

> We seem to be creating all bindings that can be accessed on the window object, even though we only need the names of the window properties, not their values

Consider this testcase:

  <script>
   delete window.Node;
   var arr = Object.getOwnPropertyNames(window);
  </script>

This needs to NOT include "Node" in arr.  The way we handle this is by actually doing the relevant lookups in getOwnPropertyNames, and the lookups are what instantiate the objects, because they have no way to know who's doing the lookup and why.

I suppose one thing we could do is only do lookups for the things that have a cached proto (and which have hence already been looked up on the window before).  That might make the first call faster, but would probably make subsequent ones slower...

Comment 7

[Boris Zbarsky [:bzbarsky]]

So the second call takes 0.2ms or so over here for me right now.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase with two calls

Comment 9

[Boris Zbarsky [:bzbarsky]]

And the non-webidl bits make this even harder, of course.  The basic problem is that right now we get the list of all the names that might be relevant, then use JS_HasUCProperty to test whether they've been deleted or not.  But JS_HasUCProperty ends up instantiating lazy stuff, via the resolve hook...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, and if you want a testcase that compares global creation between Blink and Gecko, see https://jsfiddle.net/76vqay5e/ which shows that we need about 2ms to create a global, while Chrome needs about 4 on my machine.  They used to be _way_ slower here.

In either case, Safari needs about 1ms _and_ can do fast getOwnPropertyNames, so we should really try to figure out something here.

Comment 11

[Jan de Mooij [:jandem]]

This also shows up on Speedometer v2's Angular2-TypeScript-TodoMVC-Adding100Items test. They do about 15 of these getOwnPropertyNames(window) calls but the first one is the slowest, of course (takes at least 10 ms here).

They're only doing an indexOf and don't actually care about the property value:

    function h(t, e, n) {
        for (var r = t; r && -1 === Object.getOwnPropertyNames(r).indexOf(e);)
            r = Object.getPrototypeOf(r);
        !r && t[e] && (r = t);
        var o, i = x(e);
        return r && !(o = r[i]) && (o = r[i] = r[e], r[e] = p(e, n(o, i, e))), o
    }

bz, what if we had a separate class hook to get a list of "would resolve" properties? How hard is it to implement that on the Gecko side?

There's also the JSNewEnumerateOp op, maybe we could (re)use that somehow.

Comment 12

[Jan de Mooij [:jandem]]

I'd be happy to look into this myself btw, next week or so. This could easily shave off 10-20 ms of a lot of page loads.

Comment 13

[Jan de Mooij [:jandem]]

One problem with the "just get a list of properties" approach is that it could cause inconsistent property iteration order. We already have that problem, though, due to the resolve hook...

Comment 14

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #10)
> In either case, Safari needs about 1ms _and_ can do fast
> getOwnPropertyNames, so we should really try to figure out something here.

They have a getOwnPropertyNames hook. The window implementation is here (if I'm reading the code correctly):

https://github.com/WebKit/webkit/blob/a0da9778cb6d06865a146b9161ce6b83dc857161/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp#L399

Comment 15

[Boris Zbarsky [:bzbarsky]]

> what if we had a separate class hook to get a list of "would resolve" properties? How hard is it to implement that on the Gecko side?

That's not hard to implement, depending on what "would resolve" means, but how would it solve the problem we have here?

> We already have that problem, though, due to the resolve hook

Right.

> They have a getOwnPropertyNames hook. The window implementation is here (if I'm reading the code correctly):

Right, but this doesn't seem to be doing any lazy-resolution stuff.  It's just handling the cross-origin bits, and if not delegating to Base::getOwnPropertyNames.

Comment 16

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #15)
> > what if we had a separate class hook to get a list of "would resolve" properties? How hard is it to implement that on the Gecko side?
> 
> That's not hard to implement, depending on what "would resolve" means, but
> how would it solve the problem we have here?

The problem we have here is that getOwnPropertyNames(window) et al will currently resolve all properties on the object and that's very slow because we have a ton of (DOM) prototype objects to initialize, right? So the proposal is just to add a separate hook that returns just the list of properties and using that instead of resolving everything eagerly.

Comment 17

[Boris Zbarsky [:bzbarsky]]

That hook is what window actually implements already.  Then the bindings go and look up all those props so we can avoid returning the ones that have been deleted...  That's where the resolving comes from.  So we need some way to deal with the "property got deleted" issue.  Per IRC discussion just now, we will use newenumerate and have it only return things that have NOT been resolved yet.  Need a bit more machinery on the DOM side to make this work, but should be doable.

Comment 18

[Jan de Mooij [:jandem]]

As I mentioned on IRC, we will need some way to make Object.freeze and friends work with that setup. Right now js::PreventExtensions calls js::GetPropertyKeys to force resolving all lazy properties. We could fix that by calling new-enumerate and then resolving each property it returns.

(Hopefully at some point we will be able to kill the old-enumerate hook and simplify all this...)

Comment 19

[Boris Zbarsky [:bzbarsky]]

> we will need some way to make Object.freeze and friends work with that setup. Right now js::PreventExtensions

Shouldn't be an issue.  I don't think it's possible to invoke js::PreventExtensions on a Window directly, and when invoked on a WindowProxy it won't reach the GetPropertyKeys call at all.  Instead, it will do js::Proxy::preventExtensions, which will land in nsOuterWindowProxy::preventExtensions which fails the operation as required by https://html.spec.whatwg.org/multipage/browsers.html#windowproxy-preventextensions

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Store the relevant constructor id in the entries in the WebIDLGlobalNameHash

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Add a way to ask the WebIDLGlobalNameHash for only the names that have not yet been resolved on the given window

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Switch NeedResolve bindings to using a newResolve hook instead of a resolve hook

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Add a JS friend API for getting an jsid from a known-atom JSString*

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Make getting window names a bit faster by avoiding various intermediate strings

Comment 25

[Jan de Mooij [:jandem]]

Comment on attachment 8874728 [details] [diff] [review]
part 3.  Switch NeedResolve bindings to using a newResolve hook instead of a resolve hook

Review of attachment 8874728 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +8951,5 @@
>              prefix = dedent("""
> +                // This is OK even though we're a newEnumerate hook: this will
> +                // define the relevant properties on the global, and the JS
> +                // engine will pick those up, because it looks at the object's
> +                // properties after this hook has returned.

Mind filing a follow-up bug to optimize JS_EnumerateStandardClasses? I can take it.

Comment 26

[Jan de Mooij [:jandem]]

Comment on attachment 8874730 [details] [diff] [review]
part 5.  Make getting window names a bit faster by avoiding various intermediate strings

Review of attachment 8874730 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this!

::: dom/base/nsGlobalWindow.cpp
@@ +5170,5 @@
>          // just goes ahead and recreates them.
> +        JSString* str = JS_AtomizeUCStringN(aCx,
> +                                            entry->mKey.BeginReading(),
> +                                            entry->mKey.Length());
> +        if (!str || !aNames.append(NON_INTEGER_ATOM_TO_JSID(str))) {

Did you see malloc/realloc in your profiles? Wondering if it makes sense to do aNames.reserve(x) before the loop where x is a reasonable estimate of the number of properties we expect.

::: dom/bindings/WebIDLGlobalNameHash.cpp
@@ +321,5 @@
>           !cache->EntrySlotIfExists(entry->mConstructorId)) &&
>          (!entry->mEnabled || entry->mEnabled(aCx, aObj))) {
> +      JSString* str = JS_AtomizeStringN(aCx, sNames + entry->mNameOffset,
> +                                        entry->mNameLength);
> +      if (!str || !aNames.append(NON_INTEGER_ATOM_TO_JSID(str))) {

Same here.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> Mind filing a follow-up bug to optimize JS_EnumerateStandardClasses? I can take it.

Yep, bug 1370608.

> Did you see malloc/realloc in your profiles?

A bit.  Especially before the "part 5" patch.  I just did a profile with this entire patch stack, and the summary is like so.  Of the time spent under the Object.getOwnPropertyNames call, about 53% is under the window enumerate hook (the rest is Snapshot, proxy machinery, etc).  Almost all that time is under WebIDLGlobalNameHash::GetNames, and breaks down as follows (all percentages of the getOwnPropertyNames call):

*  25% js::Atomize
*   5% hashtable iterator
*   3% Vector::growStorageBy (1% malloc(), some free(), etc).
* 0.5% BarrierMethods::exposeToJS (filed bug 1370614 on this).

and most of the last is a _very_ long tail of tests for whether the given constructor is actually supposed to be exposed on the given window.

So it doesn't look like the malloc/realloc is a huge issue at first glance.  Probably because our list has 900+ entries, so we only realloc 10 times or so as we go....

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3 updated to not expose DOMConstructor, because it's special like that

Comment 29

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8874943 [details] [diff] [review]
Part 3 updated to not expose DOMConstructor, because it's special like that

Review of attachment 8874943 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindow.cpp
@@ +5149,5 @@
> +    // return all possible WebIDL names, because we don't really support
> +    // deleting these names off our Xray; trying to resolve them will just make
> +    // them come back.  In the former case, we want to avoid returning deleted
> +    // names.  But the JS engine already knows about the non-deleted
> +    // already-resolve names, so we can just return the so-far-unresolved ones.

wording: already-resolved?

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68989f96eb93
part 1.  Store the relevant constructor id in the entries in the WebIDLGlobalNameHash.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b72b120485d
part 2.  Add a way to ask the WebIDLGlobalNameHash for only the names that have not yet been resolved on the given window.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab1e672183ad
part 3.  Switch NeedResolve bindings to using a newResolve hook instead of a resolve hook.  r=qdot,jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/79157ef8e455
part 4.  Add a JS friend API for getting an jsid from a known-atom JSString*.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d37f2bce087
part 5.  Make getting window names a bit faster by avoiding various intermediate strings.  r=qdot,jandem

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/68989f96eb93
https://hg.mozilla.org/mozilla-central/rev/4b72b120485d
https://hg.mozilla.org/mozilla-central/rev/ab1e672183ad
https://hg.mozilla.org/mozilla-central/rev/79157ef8e455
https://hg.mozilla.org/mozilla-central/rev/2d37f2bce087