1191570 - Use ToPropertyKey everywhere the standard uses it

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

We have a few bugs where the behavior of `x[y]` is wrong when y is an object whose toString() method returns a symbol.

`x[{toString() { return sym; }}]`, where sym is a symbol, should behave like
`x[sym]`. Instead we treat it as `x["undefined"]`.

Maybe the right fix is to merge ValueToId and ToPropertyKey. Looking into it.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Use ToPropertyKey everywhere ES6 says to use it

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8678254 [details] [diff] [review]
Use ToPropertyKey everywhere ES6 says to use it

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

I could have sworn somewhere today I saw evilpie comment on the badness that is JSOP_TOID/ToIdOperation also munging the object as well, in who-knows-whether-it's-the-correct-order.  I guess that can be a followup, with just swapping things in-place okay here.

(Oh, note how ToIdOperation *does not* ToObject the objval if the id isInt32().  Whaaaa?)

I'm not wholly certain all the Ion JIT spots that might need updating here, have been updated.  Might be worth running past jandem too, to be sure.

::: js/src/tests/ecma_6/Expressions/ToPropertyKey-symbols.js
@@ +44,5 @@
> +        [sym]() { return "X"; }
> +    }
> +    class Y extends X {
> +        [sym]() { return super[key]() + "Y"; }
> +    }

Needs some classesEnabled(class with extends syntax) here.

Comment 3

[André Bargull [:anba]]

Comment on attachment 8678254 [details] [diff] [review]
Use ToPropertyKey everywhere ES6 says to use it

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

::: js/src/builtin/Object.cpp
@@ +517,5 @@
>      // we can safely retrieve the object's shape.
>  
>      /* Step 1, 2. */
>      jsid id;
> +    if (args.thisv().isObject() && idValue.isString() && ValueToId<NoGC>(cx, idValue, &id)) {

I don't understand why `&& idValue.isString()` was added here.
1. Doesn't that mean `obj.hasOwnProperty(0)` now takes the slow path?
2. Shouldn't obj_hasOwnProperty and obj_propertyIsEnumerable both use the same kind of optimization?

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8678254 [details] [diff] [review]
Use ToPropertyKey everywhere ES6 says to use it

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

::: js/src/builtin/Object.cpp
@@ +517,5 @@
>      // we can safely retrieve the object's shape.
>  
>      /* Step 1, 2. */
>      jsid id;
> +    if (args.thisv().isObject() && idValue.isString() && ValueToId<NoGC>(cx, idValue, &id)) {

The optimization avoids something like half a dozen memory moves to root/unroot an id and an object, which strikes me as small fry enough to just remove it, honestly.  But it's not an argument I feel the need to argue strongly.

As for your questions, 1) yes, the "slow" path, and 2) maybe, but honestly all these operations are off the beaten path enough that no one will care if they have different perf (even assuming they have meaningfully different perf!).

Comment 5

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> I could have sworn somewhere today I saw evilpie comment on the badness that
> is JSOP_TOID/ToIdOperation also munging the object as well, in
> who-knows-whether-it's-the-correct-order.  I guess that can be a followup,
> with just swapping things in-place okay here.

I tried just deleting the badness in bug 1217945, but evilpie pointed out a bug.

> (Oh, note how ToIdOperation *does not* ToObject the objval if the id
> isInt32().  Whaaaa?)

I know...

> I'm not wholly certain all the Ion JIT spots that might need updating here,
> have been updated.  Might be worth running past jandem too, to be sure.

Yep.

> Needs some classesEnabled(class with extends syntax) here.

Done locally.

Comment 6

[Jason Orendorff [:jorendorff]]

(In reply to André Bargull from comment #3)
> >      /* Step 1, 2. */
> >      jsid id;
> > +    if (args.thisv().isObject() && idValue.isString() && ValueToId<NoGC>(cx, idValue, &id)) {
> 
> I don't understand why `&& idValue.isString()` was added here.
> 1. Doesn't that mean `obj.hasOwnProperty(0)` now takes the slow path?
> 2. Shouldn't obj_hasOwnProperty and obj_propertyIsEnumerable both use the
> same kind of optimization?

Yes. I looked closer and it's fine as-is, since ValueToId<NoGC> bails out when idValue is an object.

This is a little more precarious than I would like, but I have other things to work on...

Comment 7

[Jason Orendorff [:jorendorff]]

I also re-rewrote the comment on JSOP_ID in vm/Opcodes.h in light of the awful truth about what it does.

>      /*
> -     * Pops the top of stack value, converts it into a jsid (int, string, or
> -     * symbol), and pushes it onto the stack.
> +     * First, throw a TypeError if baseValue is null or undefined. Then,
> +     * replace the top-of-stack value propertyNameValue with
> +     * ToPropertyKey(propertyNameValue). This opcode implements ES6 12.3.2.1
> +     * steps 7-10.  It is also used to implement computed property names; in
> +     * that case, baseValue is always an object, so the first step is a no-op.
>       *   Category: Literals
>       *   Type: Object
>       *   Operands:
> -     *   Stack: val => id
> +     *   Stack: baseValue, propertyNameValue => baseValue, propertyKey
>       */ \
>      macro(JSOP_TOID,          225, "toid",         NULL,  1,  1,  1,  JOF_BYTE) \

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: Use ToPropertyKey everywhere ES6 says to use it

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 8680384 [details] [diff] [review]
Use ToPropertyKey everywhere ES6 says to use it

Carrying forward Jeff's review. r?jandem for the JITs.

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8680384 [details] [diff] [review]
Use ToPropertyKey everywhere ES6 says to use it

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

The remaining ValueToId(Pure) calls in jit/ only take strings or symbols.

I can't think of any other places where the JITs handle non-int32/string/symbol indexes.

Comment 11

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bccd2788f121ff7f5f7a963583b94e14b34f01e
Bug 1191570 - Use ToPropertyKey everywhere ES6 says to use it. r=Waldo, r=jandem.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4bccd2788f12

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/4bccd2788f12

Comment 14

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion