Closed Bug 761685 Opened 12 years ago Closed 12 years ago

IonMonkey: Add support for call objects

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dvander, Assigned: dvander)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

      No description provided.
Attached patch WIP v0, initial sketch (obsolete) — Splinter Review
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.
This patch simplifies the CallObject creation path for deep scope chains by ensuring that all cacheable shapes are delegates.
Attachment #630806 - Flags: review?(luke)
Attached patch WIP v1 (obsolete) — Splinter Review
Attachment #630434 - Attachment is obsolete: true
Attached patch wip v2 (obsolete) — Splinter Review
This version still fails some tests, but is about 4X faster than JM on a microbenchmark where call object creation dominates.
Attachment #630841 - Attachment is obsolete: true
Attached patch patchSplinter Review
Luke, could you give the relevant parts of this a glance?
Attachment #630848 - Attachment is obsolete: true
Attachment #630894 - Flags: review?(luke)
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.
Attachment #630894 - Flags: review?(jdemooij)
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.
Attachment #630894 - Flags: review?(jdemooij) → review+
(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 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.
Attachment #630806 - Flags: review?(luke) → review+
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.
Attachment #630894 - Flags: review?(luke) → review+
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).
Attachment #632091 - Flags: review?(luke)
Attachment #632091 - Flags: review?(luke) → review+
Landed non-Ion part as https://hg.mozilla.org/projects/ionmonkey/rev/82bea6f569c8 and waiting on AWFY results to land the rest.
The rest landed, right?
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.