Closed Bug 1169743 Opened 6 years ago Closed 4 years ago

Make classes with extends work in the Jits

Categories

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

Product:
Core
Component:
JavaScript Engine: JIT
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: efaust, Assigned: tcampbell)

References

(Blocks 3 open bugs)

Details

(Keywords: perf)

Attachments

(3 files, 3 obsolete files)

Bug 1169743 - Rework JSOP_CLASSHERITAGE to be jit-friendly.
59 bytes, text/x-review-board-request
shu
: review+
Details
Bug 1169743 - Implement class decls with extends in Baseline
59 bytes, text/x-review-board-request
jandem
: review+
Details
0001-Bug-1169743-Implement-class-decls-with-extends-in-Ba.patch
8.58 KB, patch
jandem
: review+
Details | Diff | Splinter Review 
No description provided.
Blocks: 1169740
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8708570 - Flags: review?(jorendorff)
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 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.
Attachment #8708572 - Flags: review?(jdemooij) → review+
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"?
Attachment #8708570 - Flags: review?(jorendorff) → review+
uhm...why didn't this land? ni? me until I figure out where I dropped this on the floor.
Flags: needinfo?(efaustbmo)
Blocks: jsperf
Priority: -- → P2
Keywords: perf
Assignee: efaustbmo → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(efaustbmo)
Priority: P2 → P1
Realistically this will be for next release.
Priority: P1 → P2
Flags: needinfo?(hv1989)
Flags: needinfo?(hv1989)
Priority: P2 → P1
Assignee: nobody → tcampbell
Attachment #8708570 - Attachment is obsolete: true
Attachment #8708572 - Attachment is obsolete: true
I've cleaned up these patches and brought them up to date. Requesting re-reviews since a number of APIs have changed.
Attachment #8871011 - Flags: review?(jdemooij)
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.
Attachment #8871011 - Flags: review?(jdemooij) → review+
Attachment #8871010 - Flags: review?(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.
Attachment #8871010 - Flags: review?(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
Attachment #8871010 - Flags: review?(shu) → review+
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.
Pushed by tcampbell@mozilla.com:
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
Keywords: leave-open
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.
Attachment #8875450 - Flags: review?(jdemooij)
Some tests still fail. Will look into them tomorrow.
Respin of patch with missing write barriers in Baseline INITHOMEOBJECT.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eab95d04774eec4e65f08784cbc012bdd3b2b622
Attachment #8875450 - Attachment is obsolete: true
Attachment #8875450 - Flags: review?(jdemooij)
Attachment #8875578 - Flags: review?(jdemooij)
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.
Attachment #8875578 - Flags: review?(jdemooij) → review+
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24191914f43b
Implement class decls with extends in Baseline (cont.) r=jandem
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.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.