Last Comment Bug 766468 - IonMonkey: Inline creation of callobjects with dynamic slots
: IonMonkey: Inline creation of callobjects with dynamic slots
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:
Blocks: IonSpeed 765980
  Show dependency treegraph
 
Reported: 2012-06-19 22:44 PDT by David Anderson [:dvander]
Modified: 2012-06-22 18:52 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (37.11 KB, patch)
2012-06-20 19:03 PDT, David Anderson [:dvander]
jdemooij: review+
Details | Diff | Splinter Review

Description David Anderson [:dvander] 2012-06-19 22:44:36 PDT
Needed for earley-boyer. This is sort of tricky because we have to call out to malloc. After playing around with a bunch of ways to do this, I think I've settled on:

 (1) Split LNewCallObject into LCallNewCallObject, LNewCallObject, and
     LNewCallObjectWithSlots.

 (2) If we determine we're going to inline callobject creation, then we'll
     move CALLEE/ENCLOSING_SCOPE slot initialization to MIR.

 (3) The out-of-line paths for NewCallObject[WithSlots] will be boiled down
     to just a JSObject::create all. The function will take three new
     parameters: gc::AllocKind, HandleTypeObject, and HeapSlot *.

 (4) Introduce MNewSlots/LNewSlots.

 (5) Make MNewCallObject ternary so it can take in an MNewSlots (or Null
     constant).

This would be somewhat ideal if allocated slots were gcthings, but they're not, so we have to be careful about not leaking the result of MNewSlots. On the other hand, the JS engine already leaks dynamic slots if object allocation fails, so maybe it's not a big deal. The truly ideal scenario would be if we didn't need two allocations to get a call object.
Comment 1 Brian Hackett (:bhackett) 2012-06-20 07:05:24 PDT
(In reply to David Anderson [:dvander] from comment #0)
> The truly ideal scenario would be if we didn't need two allocations to get a call object.

After generational GC both the object and slots should be able to be allocated out of the nursery.  This will just be a single allocation, with the obj->slots pointing to a segment of the nursery immediately following the object.
Comment 2 David Anderson [:dvander] 2012-06-20 19:03:50 PDT
Created attachment 635163 [details] [diff] [review]
patch

This is about a 5% win on earley-boyer.
Comment 3 David Anderson [:dvander] 2012-06-20 19:06:43 PDT
(In reply to Brian Hackett (:bhackett) from comment #1)
> (In reply to David Anderson [:dvander] from comment #0)
> > The truly ideal scenario would be if we didn't need two allocations to get a call object.
> 
> After generational GC both the object and slots should be able to be
> allocated out of the nursery.  This will just be a single allocation, with
> the obj->slots pointing to a segment of the nursery immediately following
> the object.

That would be great! In the meantime, maybe it's worth looking into other ways to either avoid making these call objects or keeping their slots inline.
Comment 4 Jan de Mooij [:jandem] (PTO until July 31) 2012-06-22 05:11:22 PDT
Comment on attachment 635163 [details] [diff] [review]
patch

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

::: js/src/ion/CodeGenerator.cpp
@@ +1127,4 @@
>      static const VMFunction NewCallObjectInfo = FunctionInfo<pf>(NewCallObject);
>  
> +    RootedObject templateObj(gen->cx, lir->mir()->templateObj());
> +    if (templateObj->lastProperty()->extensibleParents()) {

Nit: maybe use lir->isCall() to avoid duplicating the condition.

::: js/src/ion/IonBuilder.cpp
@@ +2987,5 @@
> +    current->add(slots);
> +
> +    // Allocate the actual object. It is important that no intervening
> +    // instructions could potentially bailout, thus leaking the dynamic slots
> +    // pointer.

If it's not too complicated, we should assert this during Lowering/Codegen. Afaik GVN won't move an instruction between these two but it'd be good to verify this.

::: js/src/ion/Lowering.cpp
@@ +160,4 @@
>  
> +    LAllocation slots;
> +    if (isCall)
> +        slots = useFixed(ins->slots(), CallTempReg2);

Do we always have dynamic slots in this case? Can we assert it?

@@ +165,5 @@
> +        slots = useRegister(ins->slots());
> +    else
> +        slots = LConstantIndex::Bogus();
> +
> +    LNewCallObject *lir = new LNewCallObject(slots);

Nit: JS_ASSERT(lir->isCall() == isCall);

::: js/src/vm/ScopeObject.cpp
@@ +85,5 @@
>   * Construct a call object for the given bindings.  If this is a call object
>   * for a function invocation, callee should be the function being called.
>   * Otherwise it must be a call object for eval of strict mode code, and callee
>   * must be null.
>   */

Nit: this comment should be updated, for instance there's no longer a callee here.
Comment 5 David Anderson [:dvander] 2012-06-22 18:49:16 PDT
(In reply to Jan de Mooij (:jandem) from comment #4)
> 
> If it's not too complicated, we should assert this during Lowering/Codegen.
> Afaik GVN won't move an instruction between these two but it'd be good to
> verify this.

I tried this, but of course it got too complicated.

> > +    LAllocation slots;
> > +    if (isCall)
> > +        slots = useFixed(ins->slots(), CallTempReg2);
> 
> Do we always have dynamic slots in this case? Can we assert it?

Err, good catch. No we don't necessarily so that's fixed now.
Comment 6 David Anderson [:dvander] 2012-06-22 18:52:18 PDT
http://hg.mozilla.org/projects/ionmonkey/rev/05a756967f25

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