Closed Bug 765454 Opened 8 years ago Closed 7 years ago

IonMonkey: Modify Bailout system to allow for directly inlining common property accessors.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: efaust, Assigned: efaust)

References

Details

(Whiteboard: [ion:p2])

Attachments

(3 files, 5 obsolete files)

Currently, the GET_ARGC() in InlineFrameIterator::findNextFrame() does a lookup in the snapshot for the number of arguments. As we are adding new potential inlining instruction sites JSOP_GETPROP and JSOP_SETPROP, the Outer ResumePoints from which those snapshots are generated should be fixed.
Blocks: 757932
Summary: IonMonkey: InlineFrameIterator numActualArgs() wrong for inlined getters and setters. → IonMonkey: Modify Bailout system to allow for directly inlining common property accessors.
Whiteboard: [ion:t] → [ion:p2]
Bug 782913 is pretty important for various projects implementing other VMs in JS, like JSIL and Shumway. Given that we seem to fall off a performance cliff under some getter/setter-related circumstances, having this bug fixed would be *very* nice.
OS: Windows 7 → All
Hardware: x86_64 → All
Assignee: general → efaustbmo
Status: NEW → ASSIGNED
Attached patch WIP (getters only) (obsolete) — Splinter Review
Attached patch Final WIP (obsolete) — Splinter Review
Needs polish before review, but this is functionally complete.
Attachment #777255 - Attachment is obsolete: true
Attached patch FinalFinal WIP (obsolete) — Splinter Review
Be less agressive about faulting in lazy scripts. Same performance, less memory insanity, with any luck.
Attachment #778126 - Attachment is obsolete: true
Attached patch FinalFinal WIP (obsolete) — Splinter Review
Now with less guaranteed assertion on native getters.
Attachment #778197 - Attachment is obsolete: true
Attachment #778209 - Attachment is obsolete: true
Attachment #778751 - Flags: review?(kvijayan)
Comment on attachment 778751 [details] [diff] [review]
Inline scripted accessors

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

Nice job!  The bulk of the comments are nits.

Yay inlined accessors!

::: js/src/ion/BaselineBailouts.cpp
@@ +367,5 @@
>      }
>  };
>  
> +static inline bool
> +isInlinableFallback(ICFallbackStub *icEntry)

Style: Capitalize this function name.

@@ +374,5 @@
> +           icEntry->isSetProp_Fallback();
> +}
> +
> +static inline void*
> +getStubReturnAddress(JSContext *cx, jsbytecode *pc)

Style: Capitalize this function name.

@@ +380,5 @@
> +    if (IsGetterPC(pc))
> +        return cx->compartment()->ionCompartment()->baselineGetPropReturnAddr();
> +    if (IsSetterPC(pc))
> +        return cx->compartment()->ionCompartment()->baselineSetPropReturnAddr();
> +    return cx->compartment()->ionCompartment()->baselineCallReturnAddr();

Would be good to ASSERT IsCallPC(pc) before the return.

@@ +692,5 @@
>              // Since the information is never read, we can just push undefined
>              // for all values.
> +            
> +            // When an accessor is inlined, the whole thing is a lie. There 
> +            // should never have been a call there. Fix the caller's stack to 

Whitespace at end of previous three lines.

@@ +997,1 @@
>      unsigned actualArgc = GET_ARGC(pc);

GET_ARGC is no longer applicable if |pc| isn't strictly a invoke opcode.

Leave |actualArgc| uninitialized here, move the GET_ARGC(pc) to the else block below.

::: js/src/ion/BaselineIC.cpp
@@ +6378,5 @@
>      masm.pushBaselineFramePtr(BaselineFrameReg, R0.scratchReg());
>  
> +    if (!tailCallVM(DoSetPropFallbackInfo, masm))
> +        return false;
> +    

Nit: whitespace in blank line.

@@ +6390,5 @@
> +
> +    // The function returned whatever it returned, but we are supposed to return the argument
> +    // Bypass Descriptor, CalleeToken, ActualArgc, and ThisV
> +    masm.loadValue(Address(BaselineStackReg, 4 * sizeof(size_t)), R0);
> +    

Nit: whitespace in blank line.

::: js/src/ion/BaselineIC.h
@@ +4544,5 @@
>      }
>  
>      class Compiler : public ICStubCompiler {
>        protected:
> +        uint32_t returnOffset_; 

Nit: whitespace at end of line.

::: js/src/ion/Ion.cpp
@@ +406,5 @@
>  void
>  IonCompartment::sweep(FreeOp *fop)
>  {
>      stubCodes_->sweep(fop);
> +    

Nit: whitespace at end of blank line.

::: js/src/ion/IonBuilder.cpp
@@ +7827,5 @@
>      if (!callInfo.init(current, 0))
>          return false;
> +
> +    // Inline if we can, otherwise, forget it and just generate a call.
> +    if(makeInliningDecision(getter, callInfo) && getter->isInterpreted()) { 

Nit: whitespace at end of line.  Also, add space between 'if' and '('.

@@ +8062,5 @@
> +        
> +        // Inline the setter if we can.
> +        if (makeInliningDecision(setter, callInfo) && setter->isInterpreted())
> +            return inlineScriptedCall(callInfo, setter);
> +        

Nit: Whitespace at end of previous two blank lines.

::: js/src/ion/IonBuilder.h
@@ +763,5 @@
>  
> +    bool isSetter() const {
> +        return setter_;
> +    }
> +    void makeSetter() {

Nit.  It's your call, but I thought 'markSetter' would be a better name for this.  Make sort of implies a more heavyweight transformatin.
Attachment #778751 - Flags: review?(kvijayan) → review+
Comment on attachment 778751 [details] [diff] [review]
Inline scripted accessors

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

I had some small nits about BaselineBailouts.cpp when passing by. No need to listen.

::: js/src/ion/BaselineBailouts.cpp
@@ +647,3 @@
>      // On the caller side this must represent like the function wasn't inlined.
>      uint32_t pushedSlots = 0;
> +    AutoValueVector callerArgSave(cx);

What about "savedCallerArgs"? Seems more natural for me...

@@ +647,4 @@
>      // On the caller side this must represent like the function wasn't inlined.
>      uint32_t pushedSlots = 0;
> +    AutoValueVector callerArgSave(cx);
> +    bool useArgSave = op == JSOP_FUNAPPLY || IsGetterPC(pc) || IsSetterPC(pc);

What about "needToSaveArgs" ?

@@ +689,5 @@
>              // correctly it must look like js_fun_apply was actually called.
>              // This means transforming the stack from |target, this, arg1, ...|
>              // to |js_fun_apply, target, this, argObject|.
>              // Since the information is never read, we can just push undefined
>              // for all values.

Isn't this the comment that should go just before the if (op == JSOP_FUNAPPLY)?
Comment on attachment 778751 [details] [diff] [review]
Inline scripted accessors

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

Not even a drive-by, just trying, with some success, even, to understand how this works. Feel free to ignore my comments. Also, \o/.

::: js/src/ion/BaselineBailouts.cpp
@@ +651,3 @@
>      if (iter.moreFrames() &&
> +        (op == JSOP_FUNCALL || op == JSOP_FUNAPPLY ||
> +         IsGetterPC(pc) || IsSetterPC(pc)))

These seem to fit on one line. However, it also seems like you should be able to say `(op == JSOP_FUNCALL || useArgSave)`.

@@ +772,5 @@
> +            // Accessors coming out of ion are inlined via a complete
> +            // lie perpetrated by the compiler internally. We just pretend
> +            // that the stack looked like a call all along. This means that
> +            // the depth is actually one *more* than expected, as there's now
> +            // a function on the stack, as well that the |this| object.

This sentence seems slightly garbled.

::: js/src/ion/IonBuilder.cpp
@@ +7827,5 @@
>      if (!callInfo.init(current, 0))
>          return false;
> +
> +    // Inline if we can, otherwise, forget it and just generate a call.
> +    if(makeInliningDecision(getter, callInfo) && getter->isInterpreted()) { 

I don't actually know how common native accessors are. If they are, it might make sense to flip this condition around.

@@ +8060,5 @@
> +        // Ensure that we know we are calling a setter in case we inline it.
> +        callInfo.makeSetter();
> +        
> +        // Inline the setter if we can.
> +        if (makeInliningDecision(setter, callInfo) && setter->isInterpreted())

I don't actually know if native accessors are common. If they are, it might make sense to flip this condition around.

::: js/src/ion/IonBuilder.h
@@ +763,5 @@
>  
> +    bool isSetter() const {
> +        return setter_;
> +    }
> +    void makeSetter() {

'MarkAsSetter', perhaps? 'MarkSetter' threw me off because of all the 'mark*' functions for gc.

::: js/src/ion/IonCompartment.h
@@ +219,5 @@
>      // Map ICStub keys to ICStub shared code objects.
>      typedef WeakValueCache<uint32_t, ReadBarriered<IonCode> > ICStubCodeMap;
>      ICStubCodeMap *stubCodes_;
>  
> +    // Keep track of offset into various baseline stub's code at return

nit: stubs'
Comment on attachment 778751 [details] [diff] [review]
Inline scripted accessors

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

::: js/src/ion/IonBuilder.cpp
@@ +7827,5 @@
>      if (!callInfo.init(current, 0))
>          return false;
> +
> +    // Inline if we can, otherwise, forget it and just generate a call.
> +    if(makeInliningDecision(getter, callInfo) && getter->isInterpreted()) { 

Actually just remove the getter->isInterpreted(). Since that is checked in makeInliningDecisions() -> canInlineTarget() !

@@ +8060,5 @@
> +        // Ensure that we know we are calling a setter in case we inline it.
> +        callInfo.makeSetter();
> +        
> +        // Inline the setter if we can.
> +        if (makeInliningDecision(setter, callInfo) && setter->isInterpreted())

Same here. setter->isInterpreted check is not needed.
(In reply to Hannes Verschore [:h4writer] from comment #10)
> > +    // Inline if we can, otherwise, forget it and just generate a call.
> > +    if(makeInliningDecision(getter, callInfo) && getter->isInterpreted()) { 
> 
> Actually just remove the getter->isInterpreted(). Since that is checked in
> makeInliningDecisions() -> canInlineTarget() !
> 
I'm pretty sure this isn't right. makeInliningDecision() has a fast return true for target->isNative() before canInlineTarget() is called, so we still need to check to make sure we don't assert at the top of inlineScriptedCall().
Oh right. I forget native calls.
Comment on attachment 778751 [details] [diff] [review]
Inline scripted accessors

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

Because everybody is commenting on this patch.. ;)

::: js/src/ion/BaselineIC.cpp
@@ +6388,5 @@
> +    // frame that we are emulating does. Again, we lie.
> +    entersStubFrame_ = true;
> +
> +    // The function returned whatever it returned, but we are supposed to return the argument
> +    // Bypass Descriptor, CalleeToken, ActualArgc, and ThisV

Nit: s/ThisV/return address

@@ +6389,5 @@
> +    entersStubFrame_ = true;
> +
> +    // The function returned whatever it returned, but we are supposed to return the argument
> +    // Bypass Descriptor, CalleeToken, ActualArgc, and ThisV
> +    masm.loadValue(Address(BaselineStackReg, 4 * sizeof(size_t)), R0);

I think this is wrong if you have a setter that modifies its argument, like this:

function setter(v) {
    v = 222;
    ... bailout ..
}

var foo = (obj.setter = 1);
print(foo);

This should print 1, not 222.
Oh btw, I'm really glad somebody is finally working on this! Also impressive work understanding and modifying the whole bailout system etc so quickly. Hope these minor drive-by comments don't discourage you or anything :)
Attached patch Fix (obsolete) — Splinter Review
(In reply to Jan de Mooij [:jandem] from comment #13)
> @@ +6389,5 @@
> > +    entersStubFrame_ = true;
> > +
> > +    // The function returned whatever it returned, but we are supposed to return the argument
> > +    // Bypass Descriptor, CalleeToken, ActualArgc, and ThisV
> > +    masm.loadValue(Address(BaselineStackReg, 4 * sizeof(size_t)), R0);
> 
> I think this is wrong if you have a setter that modifies its argument, like
> this:
> 
> function setter(v) {
>     v = 222;
>     ... bailout ..
> }
> 
> var foo = (obj.setter = 1);
> print(foo);
> 
> This should print 1, not 222.

Yeah, that's a good point. Here's an interdiff of a proposed fix. It's a little ugly, but it'll do, I hope.
Attached patch FixSplinter Review
It would be nice, I guess, if this also worked on ARM. Don't try and re-invent the wheel.
Attachment #779582 - Attachment is obsolete: true
Attachment #779590 - Flags: review?(jdemooij)
To test the fix. Ignore the names, they can be cleaned up. I am more than happy to land this as a jittest with the patch.
Comment on attachment 779590 [details] [diff] [review]
Fix

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

Nice! Please add the testcase as well, r=me with that.
Attachment #779590 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/43a95d462272
https://hg.mozilla.org/mozilla-central/rev/e4239e2408ab
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Could this have caused:

<Regression> Mozilla-Inbound - Dromaeo (CSS) - MacOSX 10.7 - 2.68%
While it's not conclusive, this try push seems to indicate that we are getting the new bad values even with the patch fully backed out. This is in line with my expectations about the performance implications of the patch

I suspect I am not the cause of the regression.

https://tbpl.mozilla.org/?tree=Try&rev=6d053dde1468
Hmm.  Do you have any idea what might be the cause, in the relevant checkin range?  :(
You need to log in before you can comment on or make changes to this bug.