Closed Bug 766468 Opened 12 years ago Closed 12 years ago

IonMonkey: Inline creation of callobjects with dynamic slots

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dvander, Assigned: dvander)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

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.
(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.
Attached patch patchSplinter Review
This is about a 5% win on earley-boyer.
Attachment #635163 - Flags: review?(jdemooij)
(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 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.
Attachment #635163 - Flags: review?(jdemooij) → review+
(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.
http://hg.mozilla.org/projects/ionmonkey/rev/05a756967f25
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.