1370608 - Add a newEnumerate-friendly version of JS_EnumerateStandardClasses

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

Right now things that do lazy standard classes have to enumerate them all whenever someone asks them for their list of properties.  This is noticeably slow (order of ~1ms on modern-ish laptop hardware) on the first call.

If we had this API, we could use it on top of bug 1364816 to improve things for window in particular.

Comment 1

[Jan de Mooij [:jandem]]

I was looking at this earlier today and it doesn't seem too hard, but I don't understand why it's okay for the newEnumerate hook on Window to ignore its enumerableOnly argument.

Things do seem to work correctly: Object.keys(window) does not return everything that Object.getOwnPropertyNames(window) returns.

Comment 2

[Jan de Mooij [:jandem]]

For window it's probably because of this if-else, where Object.keys ends up in the else-branch:

http://searchfox.org/mozilla-central/rev/d840ebd5858a61dbc1622487c1fab74ecf235e03/js/src/jsiter.cpp#385-416

(And Object.keys(window) is still slow)

Comment 3

[Jan de Mooij [:jandem]]

I can take a look at this later.

Comment 4

[Jan de Mooij [:jandem]]

Maybe nsOuterWindowProxy::getOwnEnumerablePropertyKeys should forward to Wrapper::getOwnEnumerablePropertyKeys instead of BaseProxyHandler::getOwnEnumerablePropertyKeys.

That would match what we do for ownPropertyKeys and should give us the fast behavior, but we do need to fix the enumerableOnly bit first. I'll file a bug for this tomorrow.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> but I don't understand why it's okay for the newEnumerate hook on Window to ignore its enumerableOnly argument.

Hrm.  I thought it was because all those properties were enumerable anyway, but I now see that they are NOT supposed to be enumerable.  So this is kinda working by accident.

I believe comment 2 is correct in terms of what's going on here at the moment.

Comment 4 is also correct.  I filed bug 1372371 on this enumerability stuff.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Move newEnumerate from ObjectOps to ClassOps

This moves the JSNewEnumerate hook to ClassOps, to make it easier to port code from the old hook to the new one.

Very mechanical for the most part, because most classes don't have a JSNewEnumerate hook.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Add JS_NewEnumerateStandardClasses

This adds JS_NewEnumerateStandardClasses and uses it instead of JS_EnumerateStandardClasses in js/src.

I confirmed Object.getOwnPropertyNames(this).sort() is the same as before.

I also changed PreventExtensions to force resolving lazy properties when there's a newEnumerate hook and a resolve hook, to handle Object.freeze(this) etc correctly.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Use JS_NewEnumerateStandardClasses outside js/src

I didn't convert SystemGlobalEnumerate as there are some XPConnect callers that are not trivial to convert to newEnumerate.

Maybe I'll take a stab at it later (it would be nice if we could remove the enumerate hook completely), but it's better done as a followup.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8877054 [details] [diff] [review]
Part 1 - Move newEnumerate from ObjectOps to ClassOps

r=me on the bits outside js/src

I did no read the js/src parts.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8877062 [details] [diff] [review]
Part 3 - Use JS_NewEnumerateStandardClasses outside js/src

>+                if (!EnumerateGlobal(cx, obj, properties, enumerableOnly)) {

Does the JS engine deduplicate the list we return here?  Or is that our responsibility?

The reason I ask is that the old behavior just resolved the standard classes on the global, then returned a list of names for the DOM bits, and the JS engine would pick up the standard class names itself as own props of the global and presumably ensure that we didn't end up repeating names.

But in the new setup we're just adding ids to the list from EnumerateGlobal and then from the Window bits.  We really _shouldn't_ end up with duplicate IDs, because people better not be defining WebIDL interface names that are the same as JS standard classes.  And Window is really the only consumer of this code...  But conceptually, where should the deduplication happen?  If in this code, it could use a comment explaining why we're not bothering.

r=me with that.

Comment 11

[Tom S]

Comment on attachment 8877054 [details] [diff] [review]
Part 1 - Move newEnumerate from ObjectOps to ClassOps

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

Ignore the renaming if you are going to change the name later anyway.

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ +224,5 @@
>      NPObjWrapper_DelProperty,
>      NPObjWrapper_GetProperty,
>      NPObjWrapper_SetProperty,
> +    nullptr,                    /* enumerate */
> +    NPObjWrapper_Enumerate,

_NewEnumerate

::: js/src/builtin/TypedObject.cpp
@@ +2233,5 @@
>          nullptr,        /* delProperty */                \
>          nullptr,        /* getProperty */                \
>          nullptr,        /* setProperty */                \
>          nullptr,        /* enumerate   */                \
> +        TypedObject::obj_enumerate,                      \

newEnumerate.

We should really rename the old hook like you suggested.

::: js/src/vm/EnvironmentObject.cpp
@@ +416,5 @@
> +    nullptr,    /* delProperty */
> +    nullptr,    /* getProperty */
> +    nullptr,    /* setProperty */
> +    nullptr,    /* enumerate */
> +    ModuleEnvironmentObject::enumerate

newEnumerate

::: js/src/vm/UnboxedObject.cpp
@@ +950,5 @@
>      nullptr,        /* delProperty */
>      nullptr,        /* getProperty */
>      nullptr,        /* setProperty */
>      nullptr,        /* enumerate   */
> +    UnboxedPlainObject::enumerate,

^

Comment 12

[Tom S]

Comment on attachment 8877057 [details] [diff] [review]
Part 2 - Add JS_NewEnumerateStandardClasses

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

Looks good. I am a bit surprised that we only had to make a single change to make lazy resolving of the properties work.

::: js/src/jsapi.cpp
@@ +1126,5 @@
> +            continue;
> +
> +        JSProtoKey key = table[i].key;
> +        if (key == JSProto_Null ||
> +            global->isStandardClassResolved(key) ||

Can we make this a single if statement, maybe with a short comment?

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #10)
> Does the JS engine deduplicate the list we return here?  Or is that our
> responsibility?

The JS engine does this. For each property, we call Enumerate and there we use a HashSet to check for duplicates:

http://searchfox.org/mozilla-central/rev/61054508641ee76f9c49bcf7303ef3cfb6b410d2/js/src/jsiter.cpp#97-100

Sometimes we optimize this when we know the HashSet is not necessary, but we always use the HashSet when we have a newEnumerate hook (see the outer if-statement), so we should be good.

Comment 14

[Jan de Mooij [:jandem]]

(In reply to Tom Schuster [:evilpie] from comment #11)
> Ignore the renaming if you are going to change the name later anyway.

I'll rename the ones you suggested. I'm a bit on the fence about it because in the end we do want to remove the enumerate hook and rename newEnumerate -> enumerate but I'm not sure when that will be and it should be easy to rename them back.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f30ec8ac7239
part 1 - Move newEnumerate hook from ObjectOps to ClassOps. r=evilpie,bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/51f0acbee7c9
part 2 - Add JS_NewEnumerateStandardClasses and use it in js/src. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4a0301fac3a
part 3 - Use JS_NewEnumerateStandardClasses outside js/src. r=bz

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f30ec8ac7239
https://hg.mozilla.org/mozilla-central/rev/51f0acbee7c9
https://hg.mozilla.org/mozilla-central/rev/d4a0301fac3a