1169743 - Make classes with extends work in the Jits

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Comment 1

[Eric Faust [:efaust]]

Attachment: Part 1: Rework JSOP_CLASSHERITAGE to be jit-friendly.

Comment 2

[Eric Faust [:efaust]]

Attachment: Part 2: Implement remaining JSOPs to allow class decls with extends in Baseline.

Comment 3

[Eric Faust [:efaust]]

Comment on attachment 8708572 [details] [diff] [review]
Part 2: Implement remaining JSOPs to allow class decls with extends in Baseline.

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

::: js/src/jit/BaselineCompiler.cpp
@@ +4365,5 @@
> +BaselineCompiler::emit_JSOP_FUNWITHPROTO()
> +{
> +    frame.syncStack(0);
> +
> +    masm.extractObject(frame.addressOfStackValue(frame.peek(-1)), R0.scratchReg());

This makes no sense. It worked by serendipity for me, as I test on x64. This should be |masm.unboxObject|, as written, though probably

frame.popRegsAndSync(1);
Register proto = masm.extractObject(R0, R2,scratchReg());

with the pop() below removed

or so is better.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8708572 [details] [diff] [review]
Part 2: Implement remaining JSOPs to allow class decls with extends in Baseline.

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

Nice.

::: js/src/jit/BaselineCompiler.cpp
@@ +4366,5 @@
> +{
> +    frame.syncStack(0);
> +
> +    masm.extractObject(frame.addressOfStackValue(frame.peek(-1)), R0.scratchReg());
> +    masm.loadPtr(frame.addressOfScopeChain(), R1.scratchReg());

Yeah I think popRegsAndSync instead of syncStack(0) makes sense.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8708570 [details] [diff] [review]
Part 1: Rework JSOP_CLASSHERITAGE to be jit-friendly.

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

r=me, and this was fine! From our conversation I felt I should expect something much worse. But geez, have you *seen* our frontend? This is nothing... :)

::: js/src/frontend/BytecodeEmitter.cpp
@@ +8458,5 @@
> +        // throw if heritage isn't either null or a non-generator constructor
> +        if (!emit1(JSOP_CHECKCLASSHERITAGE))
> +            return false;
> +
> +        if (!emit1(JSOP_DUP))

A little comment here showing the JS pseudocode for what we're doing here wouldn't suck. I think it's:
    base !== null ? <base, base.prototype> : <%FunctionPrototype%, null>

@@ +8471,5 @@
> +        if (!newSrcNote(SRC_IF_ELSE, &noteIndex))
> +            return false;
> +
> +        ptrdiff_t nullOffset;
> +        if (!emitJump(JSOP_IFNE, 0, &nullOffset))

`if` statements and ternary expressions use JSOP_IFEQ, so just for uniformity I'd probably emit STRICTNEQ IFEQ instead of STRICTEQ IFNE. But if that seems stupid to you, never mind. :)

@@ +8486,5 @@
> +
> +        // Our stack depth calculations are not smart enough to understand branches,
> +        // so decrement the stackDepth to account for the value pushed above.
> +        this->stackDepth--;
> +        MOZ_ASSERT(this->stackDepth == ifDepth);

Two things.

1. To my mind, this adjustment belongs after the emitJump(JSOP_GOTO). That's how emitConditionalExpression does it.

2. The comment could maybe point out that emitConditionalExpression does the same thing, and explain why we're only adjusting by 1 instead of 2, even though our if-expression produces 2 values. (It's because we are also consuming the value just below us on the stack.)

::: js/src/js.msg
@@ +106,5 @@
>  MSG_DEF(JSMSG_CANT_CALL_CLASS_CONSTRUCTOR, 0, JSEXN_TYPEERR, "class constructors must be invoked with |new|")
>  MSG_DEF(JSMSG_UNINITIALIZED_THIS,      1, JSEXN_REFERENCEERR, "|this| used uninitialized in {0} class constructor")
>  MSG_DEF(JSMSG_BAD_DERIVED_RETURN,      1, JSEXN_TYPEERR, "derived class constructor returned invalid value {0}")
>  MSG_DEF(JSMSG_NOT_SELFHOSTED,          1, JSEXN_TYPEERR, "callFunction() used on non self-hosted builtin {0}")
> +MSG_DEF(JSMSG_BAD_HERITAGE,            2, JSEXN_TYPEERR, "class heritage {0} is {1}")

"heritage" is spec-jargon. consider using "superclass" or "base class"?

Comment 6

[Eric Faust [:efaust]]

uhm...why didn't this land? ni? me until I figure out where I dropped this on the floor.

Comment 7

[Hannes Verschore [:h4writer]]

Realistically this will be for next release.

Comment 8

[Ted Campbell [:tcampbell]]

Attachment: Bug 1169743 - Rework JSOP_CLASSHERITAGE to be jit-friendly.

Initial patch by Eric Faust [:efaust].

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

Comment 9

[Ted Campbell [:tcampbell]]

Attachment: Bug 1169743 - Implement class decls with extends in Baseline

Initial patch by Eric Faust [:efaust].

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

Comment 10

[Ted Campbell [:tcampbell]]

I've cleaned up these patches and brought them up to date. Requesting re-reviews since a number of APIs have changed.

Comment 11

[Ted Campbell [:tcampbell]]

Comment on attachment 8871011 [details]
Bug 1169743 - Implement class decls with extends in Baseline

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142578/diff/1-2/

Comment 12

[Ted Campbell [:tcampbell]]

Comment on attachment 8871010 [details]
Bug 1169743 - Rework JSOP_CLASSHERITAGE to be jit-friendly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142576/diff/1-2/

Comment 13

[Ted Campbell [:tcampbell]]

Comment on attachment 8871011 [details]
Bug 1169743 - Implement class decls with extends in Baseline

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142578/diff/2-3/

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8871011 [details]
Bug 1169743 - Implement class decls with extends in Baseline

https://reviewboard.mozilla.org/r/142578/#review147890

Looks good, sorry for the delay.

Comment 15

[Shu-yu Guo [:shu]]

Comment on attachment 8871010 [details]
Bug 1169743 - Rework JSOP_CLASSHERITAGE to be jit-friendly.

https://reviewboard.mozilla.org/r/142576/#review149854

Looks good generally. Would like to see new version with stack comments for the BCE codegen.

::: js/src/frontend/BytecodeEmitter.cpp:10680
(Diff revision 2)
> +        IfThenElseEmitter ifThenElse(this);
> +
>          if (!emitTree(heritageExpression))
>              return false;
> -        if (!emit1(JSOP_CLASSHERITAGE))
> +
> +        // throw if heritage isn't either null or a non-generator constructor

Awkward sentence, just "Heritage must be null or a non-generator constructor" would be clearer.

::: js/src/frontend/BytecodeEmitter.cpp:10684
(Diff revision 2)
> -        if (!emit1(JSOP_CLASSHERITAGE))
> +
> +        // throw if heritage isn't either null or a non-generator constructor
> +        if (!emit1(JSOP_CHECKCLASSHERITAGE))
>              return false;
> -        if (!emit1(JSOP_OBJWITHPROTO))
> +
> +        // [IF] (heritage !== null)

The convention in BCE is to list the pseudo-code algorithm in a block comment above the emitted code, and for each emitted bytecode, document the stack state to the right.

See something like emitDestructuringOpsArray for a thorough example.

::: js/src/vm/Interpreter.cpp:975
(Diff revision 2)
>      MOZ_ASSERT(v.isSymbol());
>      return JSTYPE_SYMBOL;
>  }
>  
> +bool
> +js::CheckClassHeritage(JSContext* cx, HandleValue heritage)

Because of the error reporting behavior where it tries to decompile an on-stack value, please name this CheckClassHeritageOperation.

::: js/src/vm/Interpreter.cpp:978
(Diff revision 2)
>  
> +bool
> +js::CheckClassHeritage(JSContext* cx, HandleValue heritage)
> +{
> +    if (heritage.isObject()) {
> +        if (!heritage.toObject().isConstructor()) {

!IsConstructor(heritage) is nicer.

::: js/src/vm/Interpreter.cpp:4139
(Diff revision 2)
>  }
>  END_CASE(JSOP_ARRAYPUSH)
>  
> -CASE(JSOP_CLASSHERITAGE)
> +CASE(JSOP_CHECKCLASSHERITAGE)
>  {
> -    ReservedRooted<Value> val(&rootValue0, REGS.sp[-1]);
> +    ReservedRooted<Value> heritage(&rootValue0, REGS.sp[-1]);

stackHandleAt

::: js/src/vm/Interpreter.cpp:4150
(Diff revision 2)
>  
> -        funcProto = &val.toObject();
> -
> -        if (!GetProperty(cx, funcProto, funcProto, cx->names().prototype, &objProto))
> -            goto error;
> -
> +CASE(JSOP_BUILTINPROTO)
> +{
> +    ReservedRooted<JSObject*> builtin(&rootObject0);
> +    JSProtoKey key = static_cast<JSProtoKey>(GET_UINT8(REGS.pc));
> +    MOZ_ASSERT(key < JSProto_LIMIT);

I admit I'm not clear on the C++ enum semantics here. Should this assert be checked on the bare uint8 argument with JSProto_LIMIT cast to uint8? That is -- I'm worried by casting to JSProtoKey first, if the C++ standard permits the compiler to clamp to values <= JSProto_LIMIT.

Comment 16

[Ted Campbell [:tcampbell]]

Comment on attachment 8871010 [details]
Bug 1169743 - Rework JSOP_CLASSHERITAGE to be jit-friendly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142576/diff/2-3/

Comment 17

[Ted Campbell [:tcampbell]]

Comment on attachment 8871011 [details]
Bug 1169743 - Implement class decls with extends in Baseline

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142578/diff/3-4/

Comment 18

[Ted Campbell [:tcampbell]]

Comment on attachment 8871010 [details]
Bug 1169743 - Rework JSOP_CLASSHERITAGE to be jit-friendly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142576/diff/3-4/

Comment 19

[Ted Campbell [:tcampbell]]

Comment on attachment 8871011 [details]
Bug 1169743 - Implement class decls with extends in Baseline

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142578/diff/4-5/

Comment 20

[Shu-yu Guo [:shu]]

Comment on attachment 8871010 [details]
Bug 1169743 - Rework JSOP_CLASSHERITAGE to be jit-friendly.

https://reviewboard.mozilla.org/r/142576/#review150400

::: js/src/frontend/BytecodeEmitter.cpp:10706
(Diff revision 4)
> +    //     if defined <BaseExpression> {
> +    //       cons = DefaultDerivedConstructor(proto=homeObject, funProto=funProto);
> +    //     } else {
> +    //       cons = DefaultConstructor(proto=homeObject);
> +    //     }
> +    //   }

Awesome comment.

::: js/src/frontend/BytecodeEmitter.cpp:10802
(Diff revision 4)
> -            if (!emitAtomOp(name, JSOP_CLASSCONSTRUCTOR))
> +            if (!emitAtomOp(name, JSOP_CLASSCONSTRUCTOR))       // ... HOMEOBJ CONSTRUCTOR
>                  return false;
>          }
>      }
>  
> -    if (!emit1(JSOP_SWAP))
> +    if (!emit1(JSOP_SWAP))                                          // ... CONS HOBJ

Consistency trumps staying within the 100 column limits here imo. Please keep using CONSTRUCTOR and HOMEOBJ and just go over the max width.

::: js/src/vm/Interpreter.cpp:977
(Diff revision 4)
>  }
>  
> +bool
> +js::CheckClassHeritageOperation(JSContext* cx, HandleValue heritage)
> +{
> +    if (heritage.isObject()) {

This would return true for non-constructor objects.

if (heritage.isNull())
    return true;

if (IsConstructor(heritage))
    return true;

report(...);
return false;

::: js/src/vm/Opcodes.h:494
(Diff revision 4)
>       *   Operands:
>       *   Stack: callee, this, args => rval
>       */ \
>      macro(JSOP_STRICTSPREADEVAL,      50, "strict-spreadeval", NULL,         1,  3,  1, JOF_BYTE|JOF_INVOKE|JOF_TYPESET|JOF_CHECKSTRICT) \
>      /*
> -     * Writes the [[Prototype]] objects for both a class and its .prototype to
> +     * Ensure the result of a class's heritage expression is either null or a constructor.

Nit: Ensures

Comment 21

[Shu-yu Guo [:shu]]

Comment on attachment 8871010 [details]
Bug 1169743 - Rework JSOP_CLASSHERITAGE to be jit-friendly.

https://reviewboard.mozilla.org/r/142576/#review150406

Great comments, thanks for adding the stack state comments to parts that didn't have them before.

r=me with the logic in CheckHeritageOperation fixed.

Comment 22

[Ted Campbell [:tcampbell]]

Comment on attachment 8871010 [details]
Bug 1169743 - Rework JSOP_CLASSHERITAGE to be jit-friendly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142576/diff/4-5/

Comment 23

[Ted Campbell [:tcampbell]]

Comment on attachment 8871011 [details]
Bug 1169743 - Implement class decls with extends in Baseline

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142578/diff/5-6/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bc64157bced9
Rework JSOP_CLASSHERITAGE to be jit-friendly. r=shu
https://hg.mozilla.org/integration/autoland/rev/c1ae34baf77a
Implement class decls with extends in Baseline r=jandem

Comment 25

[Ted Campbell [:tcampbell]]

Attachment: 0001-Bug-1169743-Implement-class-decls-with-extends-in-Ba.patch

This patch adds three more opcodes needed to support derived classes (and default constructors) in Baseline. With this, top-level code that defines classes is JIT-able. Individual methods that use |super| will be supported in other bugs.

Comment 26

[Ted Campbell [:tcampbell]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2825ed1b8b7261bca55302c635b58592b7f0df13

Comment 27

[Ted Campbell [:tcampbell]]

Some tests still fail. Will look into them tomorrow.

Comment 28

[Ted Campbell [:tcampbell]]

Attachment: 0001-Bug-1169743-Implement-class-decls-with-extends-in-Ba.patch

Respin of patch with missing write barriers in Baseline INITHOMEOBJECT.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eab95d04774eec4e65f08784cbc012bdd3b2b622

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bc64157bced9
https://hg.mozilla.org/mozilla-central/rev/c1ae34baf77a

Comment 30

[Jan de Mooij [:jandem]]

Comment on attachment 8875578 [details] [diff] [review]
0001-Bug-1169743-Implement-class-decls-with-extends-in-Ba.patch

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

Great to have these ops finally implemented in Baseline.

::: js/src/jit/BaselineCompiler.cpp
@@ +4729,5 @@
> +    masm.unboxObject(frame.addressOfStackValue(frame.peek(-1)), func);
> +
> +    // Set HOMEOBJECT_SLOT
> +    Address addr(func, FunctionExtended::offsetOfMethodHomeObjectSlot());
> +    masm.guardedCallPreBarrier(addr, MIRType::Value);

Is HOMEOBJECT_SLOT always undefined at this point? If yes we don't need a pre-barrier and could assert this in debug builds.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24191914f43b
Implement class decls with extends in Baseline (cont.) r=jandem

Comment 32

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

https://hg.mozilla.org/mozilla-central/rev/24191914f43b

Comment 33

[Ted Campbell [:tcampbell]]

Closing bug as this is fixed for Baseline and seems stable in nightly. I'll open a new bug to discuss Ion support of classes.

Comment 34

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.