The default bug view has changed. See this FAQ.

IonMonkey: Inline creation of callobjects with dynamic slots

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 2

5 years ago
Created attachment 635163 [details] [diff] [review]
patch

This is about a 5% win on earley-boyer.
Attachment #635163 - Flags: review?(jdemooij)
(Assignee)

Updated

5 years ago
Blocks: 765980
(Assignee)

Comment 3

5 years ago
(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+
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
http://hg.mozilla.org/projects/ionmonkey/rev/05a756967f25
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.