761685 - IonMonkey: Add support for call objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: WIP v0, initial sketch

Doesn't do anything yet. Builds MIR for creating a call object and storing formals to its slots. Probably the right thing to do is to split the MIR into two LIR: if the callobj needs dslots, we can do a callVM. Otherwise, we can inline callobj creation.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1: make all cacheable scope objects into delegates

This patch simplifies the CallObject creation path for deep scope chains by ensuring that all cacheable shapes are delegates.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: WIP v1

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: wip v2

This version still fails some tests, but is about 4X faster than JM on a microbenchmark where call object creation dominates.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

Luke, could you give the relevant parts of this a glance?

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 630894 [details] [diff] [review]
patch

Jan, could you review the IonMonkey pieces? In particular I changed the very start of MIR construction which is a tricky area, it seems to work but there may be edge cases around recursion/recompilation checks.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 630894 [details] [diff] [review]
patch

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

r=me on the IonMonkey part with nits addressed.

::: js/src/ion/Bailouts.cpp
@@ +92,5 @@
>      }
>  }
>  
> +void
> +StackFrame::initFromBailout(JSContext *cx, SnapshotIterator &iter)

Good idea.

@@ +497,5 @@
>      return true;
>  }
>  
> +bool
> +ion::EnsureHasCallObject(JSContext *cx, StackFrame *fp)

Would it make sense to call this function from ConvertFrames or something and make it static so that we don't need to call it in EnterIon and ThunkToInterpreter?

::: js/src/ion/Ion.cpp
@@ -871,5 @@
> -    if (fp->isFunctionFrame() && fp->isStrictEvalFrame() && fp->hasCallObj()) {
> -        // Functions with call objects aren't supported yet. To support them,
> -        // we need to fix bug 659577 which would prevent aliasing locals to
> -        // stack slots.
> -        IonSpew(IonSpew_Abort, "frame has callobj");

\o/

::: js/src/ion/IonBuilder.cpp
@@ +2871,5 @@
> +IonBuilder::copyFormalIntoCallObj(MDefinition *callObj, MDefinition *slots, unsigned formal)
> +{
> +    MDefinition *param = current->getSlot(info().argSlot(formal));
> +    if (slots)
> +        current->add(MStoreSlot::New(slots, formal, param));

CallObject::setArg uses RESERVED_SLOTS + formal in both cases. Looking at ObjectImpl::getSlotAddressUnchecked this seems okay only if RESERVED_SLOTS == numFixedSlots()? Probably deserves a comment.

::: js/src/ion/MIR.h
@@ +4376,5 @@
>  };
>  
> +class MNewCallObject :
> +  public MBinaryInstruction,
> +  public MixPolicy<ObjectPolicy<0>, ObjectPolicy<1> >

AFAICS both operands are statically known to have MIRType_Object, so can we drop the type policy and assert this in the constructor?

::: js/src/ion/shared/Lowering-shared-inl.h
@@ +159,5 @@
>  
>  template <size_t Defs, size_t Ops, size_t Temps> bool
>  LIRGeneratorShared::defineVMReturn(LInstructionHelper<Defs, Ops, Temps> *lir, MDefinition *mir)
>  {
> +    lir->setMir(mir);

Why this change?

::: js/src/vm/ScopeObject.h
@@ +116,5 @@
>  {
>      static const uint32_t CALLEE_SLOT = 1;
>  
> +  public:
> +    // This function is internal and should only be used by JITs.

Nit: /* */ comments here.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Jan de Mooij (:jandem) from comment #7)
> > +bool
> > +ion::EnsureHasCallObject(JSContext *cx, StackFrame *fp)
> 
> Would it make sense to call this function from ConvertFrames or something
> and make it static so that we don't need to call it in EnterIon and
> ThunkToInterpreter?

We can't, unfortunately, because we can't GC inside bailouts. Here's a few alternate solutions:
 (1) Introduce a new "prologue" opcode (I'm becoming less in favor of this though)
 (2) Allow GC within bailouts (kind of tricky)
 (3) Perform the test + initialization from within BailoutTail
 (4) Move argument checks somewhere else to remove the problem

> > +class MNewCallObject :
> > +  public MBinaryInstruction,
> > +  public MixPolicy<ObjectPolicy<0>, ObjectPolicy<1> >
> 
> AFAICS both operands are statically known to have MIRType_Object, so can we
> drop the type policy and assert this in the constructor?

Ok.

> >  template <size_t Defs, size_t Ops, size_t Temps> bool
> >  LIRGeneratorShared::defineVMReturn(LInstructionHelper<Defs, Ops, Temps> *lir, MDefinition *mir)
> >  {
> > +    lir->setMir(mir);
> 
> Why this change?

It checks isCall early on, and the lir uses the mir to see whether isCall should return true.

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 630806 [details] [diff] [review]
part 1: make all cacheable scope objects into delegates

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

Looks great.

::: js/src/vm/ScopeObject-inl.h
@@ +30,3 @@
>  {
> +    JS_ASSERT_IF(obj->isCall() || obj->isDeclEnv() || obj->isBlock(),
> +                 obj->isDelegate());

Perhaps replace the antecedent can be replaced by IsCacheableNonGlobalScope and then give a comment saying (1) why it needs to be a delegate and (2) how we ensure this.

Comment 10

[Luke Wagner [:luke]]

Comment on attachment 630894 [details] [diff] [review]
patch

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

Cool.  It's nice to see how the IM code looks; I don't really understand it but from a high-level it looks very neat and readable.

::: js/src/ion/Bailouts.cpp
@@ +92,5 @@
>      }
>  }
>  
> +void
> +StackFrame::initFromBailout(JSContext *cx, SnapshotIterator &iter)

How bad would it be to move this definition to Stack.cpp?  I've really appreciated having everything altogether after the old bad days (you may remember the 6-file spread of (jsinterp|jscntxt)(inlines.h|.h|.cpp)).  For one, it makes it easier to see patterns for the purpose of refactoring or trying understand all the different ways a frame is created.  Other times, I'll be making some fix/change (adding a flag or field) and I usually just grep the file for uses of the relevant field.  Lastly, it's kindof annoying when you look for a definition in the .cpp and don't find it.

::: js/src/vm/Stack.h
@@ +481,5 @@
>      bool jitStrictEvalPrologue(JSContext *cx);
>  
> +    /* Called from IonMonkey to transition from bailouts. */
> +    void initFromBailout(JSContext *cx, ion::SnapshotIterator &iter);
> +    bool initCallObject(JSContext *cx);

Fortunately, there is already a function (a little lower, next to prologue()) called jitHeavyweightFunctionPrologue()) that you can reuse instead of adding initCallObject.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: mystery extra bug

The property cache test takes an in-out parameter, but mutates it even if the property cache test fails. This seems like an existing bug but only triggered with my patch queue (maybe because testing succeeds more now that objects are delegates).

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Landed non-Ion part as https://hg.mozilla.org/projects/ionmonkey/rev/82bea6f569c8 and waiting on AWFY results to land the rest.

Comment 13

[Paul Wright]

The rest landed, right?