Closed Bug 723333 Opened 12 years ago Closed 12 years ago

IonMonkey: Handle JSOP_NEW without callVM().

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sstangl, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Bug 701962 implemented support for JSOP_NEW via callVM(), in order to establish a regression base. This bug is for implementing fast-path support exclusively.

The plan is to keep around only a single version of every function, recompiling if a previously-known non-constructor is called in the context of construction. A script compiled as a constructor will be able to be called as a non-constructor with minor overhead.
Blocks: IonSpeed
Attached patch Call constructors, v1 (obsolete) — Splinter Review
Permits Ion compilation of constructors, and calling them from Ion code or the interpreter. Except in the case of InvokeConstructor(), allocation is always performed caller-side, meaning that Ion functions do not have to be be specially compiled as constructors to function as such. I would like to eventually replace this with a per-function ConstructThis shim, attached to the IonScript, that can be called directly to get the same behavior without the per-callsite codesize hit / additional type dependency.

Major optimizations, such as inline allocation, are not present. Simply compiling and calling compiled constructors improves v8 by 10%. Much work still to be done, but after this lands.
Attachment #605624 - Flags: review?(dvander)
The ARM port is incomplete; it can be ignored for the moment. Segfaults mysteriously; waiting to talk to mjrosenb.
Attached patch ARM fix for v1 patch. (obsolete) — Splinter Review
ARM is fixed. Problem was a PostIndex setting, which accidentally incremented %sp by 44 instead of 4.
Comment on attachment 605624 [details] [diff] [review]
Call constructors, v1

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

Almost dont reviewing, initial comments:

::: js/src/ion/CodeGenerator.cpp
@@ +945,5 @@
> +{
> +    Register calleeReg = ToRegister(lir->getCallee());
> +    Register protoReg  = ToRegister(lir->getPrototype());
> +
> +    // If inline allocation fails, create a new |this| using VM helpers.

We don't actually do this yet, so this comment should be removed.

::: js/src/ion/IonBuilder.cpp
@@ +2449,5 @@
> +IonBuilder::createThisScripted(MDefinition *callee)
> +{
> +    // Get callee.prototype.
> +    MCallGetProperty *getProto =
> +        MCallGetProperty::New(callee, cx->runtime->atomState.classPrototypeAtom);

It's really important to note here - or somewhere - that MCallGetProperty in this case *must* be idempotent. (It could probably be MGetPropertyCache actually.) It does not have its own bytecode, and therefore we can't call resumeAfter(). But it *can* invalidate, since it can GC.

::: js/src/ion/IonFrames.h
@@ +364,4 @@
>  void MarkIonActivations(JSRuntime *rt, JSTracer *trc);
>  
>  static inline uint32
> +MakeFrameDescriptor(uint32 frameSize, FrameType type, bool constructing)

Do we need the constructing bit on the descriptor? What if we just read caller pc (and for the entry case, use the original fp's constructing bit? It might make things way simpler re: rectifier frames. If the caller is Ion code we just use IonFrameIterator to get the prev pc.

::: js/src/ion/IonMacroAssembler.cpp
@@ +158,5 @@
>      freeStack(new_diff);
>  }
> +
> +void
> +MacroAssembler::getNewObject(JSContext *cx, JSObject *templateObject,

It looks like this isn't used or working yet, so I'd leave it out of this patch (same for its entry in the header).

::: js/src/ion/IonMacroAssembler.h
@@ +71,4 @@
>          AutoRooter(JSContext *cx, MacroAssembler *masm)
>            : AutoGCRooter(cx, IONMASM),
>              masm_(masm)
> +        { }

uber-nit: I actually prefer the expanded style because it's easier to step through in gdb, maybe it stopped having those problems though.

::: js/src/ion/Lowering.cpp
@@ +185,5 @@
> +    LCreateThis *lir = new LCreateThis(useFixed(ins->getCallee(), CallTempReg0),
> +                                       useFixed(ins->getPrototype(), CallTempReg1));
> +    
> +    // Boxed for passing the argument.
> +    // TODO: If we're inlining constructors, this isn't the case.

This :TODO: can be removed.

@@ +207,5 @@
> +                tempFixed(CallTempReg1), tempFixed(CallTempReg2), tempFixed(CallTempReg3));
> +        if (!defineReturn(lir, call))
> +            return false;
> +        if (!assignSafepoint(lir, call))
> +            return false;

Can the assignSafepoint call be hoisted back out of these ifs?

::: js/src/ion/MIR.h
@@ +1424,5 @@
> +    MDefinition *getPrototype() const {
> +        return getOperand(1);
> +    }
> +    bool hasTemplateObject() const {
> +        return (templateObject_ != NULL);

!!templateObject_

@@ +1431,5 @@
> +        return templateObject_;
> +    }
> +
> +    // Although creation of |this| is effectful, it is safely repeatable.
> +    // TODO: I think this permits hoisting, though, which would be wrong.

Is it effectful? It shouldn't be. I think the alias set is right.

@@ +3568,5 @@
> +
> +    // Constructors need to perform a GetProp on the function prototype.
> +    // Since getters cannot be set on the prototype, fetching is non-effectful.
> +    // The operation may be safely repeated in case of bailout.
> +    void markUneffectful() {

This boolean bit is a little gross. Would it be cleaner to derive a new class that uses the same opcode? That or rename the bit to "mightHaveGetter_" or something.

::: js/src/ion/VMFunctions.cpp
@@ +77,3 @@
>      bool ok = InvokeConstructor(cx, fval, argc, argvWithoutThis, rval);
> +
> +    JS_ASSERT(!rval->isPrimitive());

See CallJSNativeConstructor - natives can return primitives, so if we want to assert it has to be really elaborate. Better to just remove it here.

::: js/src/ion/x64/Trampoline-x64.cpp
@@ +69,2 @@
>  #if defined(_WIN64)
> +    // TODO: All these values are wrong. Need to test.

Might want to ping Makoto Kato to let him know this might break.

::: js/src/jsinterp.cpp
@@ +1846,5 @@
> +
> +            if (regs.fp()->isConstructing() && interpReturnOK &&
> +                regs.fp()->returnValue().isPrimitive())
> +            {
> +                regs.fp()->setReturnValue(ObjectValue(regs.fp()->constructorThis()));

This should be handled in EnterIon.

@@ +2839,5 @@
> +            if (regs.fp()->isConstructing() && interpReturnOK &&
> +                regs.fp()->returnValue().isPrimitive())
> +            {
> +                regs.fp()->setReturnValue(ObjectValue(regs.fp()->constructorThis()));
> +            }

Ditto.
- Constructor bit inferred at bailout time
- ARM fixed
- fix |this| being DCE'd
- fix random bug in EarleyBoyer with OSR putting an MPassArg in an MResumePoint

Passes all tests. Small improvement to benchmarks, mostly binary-trees.
Will post the diff from the first patch in a second.
Attachment #605624 - Attachment is obsolete: true
Attachment #606040 - Attachment is obsolete: true
Attachment #605624 - Flags: review?(dvander)
Attachment #607758 - Flags: review?(dvander)
Comment on attachment 607759 [details] [diff] [review]
Diff of patch-v2 against patch-v1.

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

::: js/src/ion/Bailouts.cpp
@@ +336,4 @@
>  
>      br->setEntryFrame(fp);
>  
> +    // Derive the constructing bit from the pc of the caller.

Better if this logic can be extracted out into a separate function to keep this one shorter.

@@ +347,5 @@
> +
> +        if (fiter.type() == IonFrame_JS) {
> +            // In the case of a JS frame, look up the pc from the snapshot.
> +            InlineFrameIterator ifi = InlineFrameIterator(&fiter);
> +            JS_ASSERT((JSOp)*ifi.pc() == JSOP_NEW || (JSOp)*ifi.pc() == JSOP_CALL);

This should be:
js_CodeSpec[*ifi.pc()].flags & JOF_INVOKE

@@ +365,5 @@
> +
> +              // Either JSOP_LOOPENTRY, or Ion invocation via RunScript().
> +              default:
> +                if (entryFp->isConstructing())
> +                    fp->setConstructing();

I think it's valid to always just use entryFp->isConstructing() here, so you should be able to remove this pc snooping.

::: js/src/ion/IonBuilder.cpp
@@ +256,5 @@
>  
> +    // Wrap |this| with a guaranteed use, to prevent instruction elimination.
> +    if (info().fun()) {
> +        MThisValue *thisval = MThisValue::New(current->getSlot(info().thisSlot()));
> +        current->add(thisval);

Alternately if you don't like MThisValue, you could just set the isGuard bit or something.

@@ +2937,5 @@
> +    // Wrap |this| with a guaranteed use, to prevent instruction elimination.
> +    if (info().fun()) {
> +        MThisValue *thisval = MThisValue::New(preheader->getSlot(info().thisSlot()));
> +        preheader->add(thisval);
> +    }

In IonAnalysis.cpp, you will need a special case that a phi of the |this| slot is never removed.

::: js/src/ion/Lowering.cpp
@@ +224,4 @@
>          if (!target && !assignSnapshot(lir))
>              return false;
>  
> +        lirins = (LInstruction *)lir;

Urgh... sorry, I didn't realize you'd have to do this. Feel free to revert if you want.

::: js/src/ion/MIR.h
@@ +3304,5 @@
> +class MThisValue
> +  : public MUnaryInstruction
> +{
> +    MThisValue(MDefinition *thisval)
> +      : MUnaryInstruction(thisval)

Thinking about it more, I like the setGuard idea now, since here it's not clear how to make MThisValue return the correct type.

::: js/src/jsinterp.cpp
@@ +466,5 @@
> +            if (fp->isConstructing() && fp->functionThis().isPrimitive()) {
> +                JSObject *obj = js_CreateThis(cx, &fp->callee());
> +                if (!obj)
> +                    return false;
> +                fp->functionThis().setObject(*obj);

Hrmmmmmm. This needs to run in the ion::Cannon case under JSOP_CALL as well. Maybe we should just call ScriptPrologue here and there instead. But that's not quite right either because then it would be impossible to balance the Probes:: hook.

My vote is to just move this into ion::Cannon - better if you can use js_CreateThisForFunction.

::: js/src/vm/Stack.cpp
@@ +1173,4 @@
>              }
>  
>  #ifdef JS_ION
> +            if (fp_->runningInIon() && ionActivations_.top()) {

This second condition is a little misleading - we could have nested activations but still be inside ::Bailout. I'd just remove it. OTOH it should be valid to NULL ionTop inside ::Bailout to make sure no one can access it, but really using StackIter inside ::Bailout is just not allowed.
Comment on attachment 607758 [details] [diff] [review]
Call constructors, v2

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

r=me with above addressed
Attachment #607758 - Flags: review?(dvander) → review+
https://hg.mozilla.org/projects/ionmonkey/rev/5108b08c2d54

Still breaks Win64 (trips an assertion), but that can be fixed elsewhere. Ideally I will install Visual Studio on a Windows computer at some point.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.