Last Comment Bug 761685 - IonMonkey: Add support for call objects
: IonMonkey: Add support for call objects
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: David Anderson [:dvander]
:
Mentors:
Depends on: 659577 764792
Blocks: IonSpeed 743394 762414 762421
  Show dependency treegraph
 
Reported: 2012-06-05 10:08 PDT by David Anderson [:dvander]
Modified: 2012-07-03 00:25 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP v0, initial sketch (15.19 KB, patch)
2012-06-05 21:32 PDT, David Anderson [:dvander]
no flags Details | Diff | Review
part 1: make all cacheable scope objects into delegates (9.03 KB, patch)
2012-06-06 19:01 PDT, David Anderson [:dvander]
luke: review+
Details | Diff | Review
WIP v1 (37.90 KB, patch)
2012-06-06 21:35 PDT, David Anderson [:dvander]
no flags Details | Diff | Review
wip v2 (40.80 KB, patch)
2012-06-06 22:15 PDT, David Anderson [:dvander]
no flags Details | Diff | Review
patch (46.20 KB, patch)
2012-06-07 02:08 PDT, David Anderson [:dvander]
luke: review+
jdemooij: review+
Details | Diff | Review
mystery extra bug (1.08 KB, patch)
2012-06-11 17:54 PDT, David Anderson [:dvander]
luke: review+
Details | Diff | Review

Description David Anderson [:dvander] 2012-06-05 10:08:32 PDT

    
Comment 1 David Anderson [:dvander] 2012-06-05 21:32:56 PDT
Created attachment 630434 [details] [diff] [review]
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] 2012-06-06 19:01:41 PDT
Created attachment 630806 [details] [diff] [review]
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] 2012-06-06 21:35:44 PDT
Created attachment 630841 [details] [diff] [review]
WIP v1
Comment 4 David Anderson [:dvander] 2012-06-06 22:15:55 PDT
Created attachment 630848 [details] [diff] [review]
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] 2012-06-07 02:08:34 PDT
Created attachment 630894 [details] [diff] [review]
patch

Luke, could you give the relevant parts of this a glance?
Comment 6 David Anderson [:dvander] 2012-06-07 02:10:08 PDT
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] 2012-06-07 05:44:27 PDT
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] 2012-06-07 14:30:06 PDT
(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] 2012-06-07 22:29:34 PDT
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] 2012-06-07 23:12:43 PDT
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] 2012-06-11 17:54:32 PDT
Created attachment 632091 [details] [diff] [review]
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] 2012-06-13 20:38:29 PDT
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 2012-06-14 12:27:28 PDT
The rest landed, right?

Note You need to log in before you can comment on or make changes to this bug.