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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

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.

Comment 1

[Till Schneidereit [:till]]

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.

Comment 2

[Eric Faust [:efaust]]

Attachment: WIP (getters only)

Comment 3

[Eric Faust [:efaust]]

Attachment: Final WIP

Needs polish before review, but this is functionally complete.

Comment 4

[Eric Faust [:efaust]]

Attachment: FinalFinal WIP

Be less agressive about faulting in lazy scripts. Same performance, less memory insanity, with any luck.

Comment 5

[Eric Faust [:efaust]]

Attachment: FinalFinal WIP

Now with less guaranteed assertion on native getters.

Comment 6

[Eric Faust [:efaust]]

Attachment: Inline scripted accessors

Comment 7

[Kannan Vijayan [:djvj]]

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.

Comment 8

[Hannes Verschore [:h4writer]]

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 9

[Till Schneidereit [:till]]

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 10

[Hannes Verschore [:h4writer]]

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.

Comment 11

[Eric Faust [:efaust]]

(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().

Comment 12

[Hannes Verschore [:h4writer]]

Oh right. I forget native calls.

Comment 13

[Jan de Mooij [:jandem]]

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.

Comment 14

[Jan de Mooij [:jandem]]

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 :)

Comment 15

[Eric Faust [:efaust]]

Attachment: Fix

(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.

Comment 16

[Eric Faust [:efaust]]

Attachment: Fix

It would be nice, I guess, if this also worked on ARM. Don't try and re-invent the wheel.

Comment 17

[Eric Faust [:efaust]]

Attachment: Setter argument set bailout test.

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 18

[Jan de Mooij [:jandem]]

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.

Comment 19

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43a95d462272

"whoops, forgot the test"
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4239e2408ab

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/43a95d462272
https://hg.mozilla.org/mozilla-central/rev/e4239e2408ab

Comment 21

[Boris Zbarsky [:bzbarsky]]

Could this have caused:

<Regression> Mozilla-Inbound - Dromaeo (CSS) - MacOSX 10.7 - 2.68%

Comment 22

[Eric Faust [:efaust]]

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

Comment 23

[Boris Zbarsky [:bzbarsky]]

Hmm.  Do you have any idea what might be the cause, in the relevant checkin range?  :(