796114 - IonMonkey: Inline calls with type barriers.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

Attachment: Inline with type-checked arguments.

+++ This bug was initially created as a clone of Bug #777583 +++

I compared the master branch with attachment 657969 [details] [diff] [review] (duplicated here for review) plus attachment 663363 [details] [diff] [review] (landed) making sure we don't inline everything.  Each sunspider benchmark were run 5000 times and each kraken benchmark were run 500 times on a Intel(R) Core(TM) i7-2820QM CPU @ 2.30GHz.

            sunspider               kraken
master:  190.5ms +/- 0.0%       1841.2ms +/- 0.1%
     1:  197.6ms +/- 0.0%       1859.2ms +/- 0.1%
     2:  195.1ms +/- 0.0%       1818.0ms +/- 0.1%
     4:  194.1ms +/- 0.0%       1818.0ms +/- 0.1%
     6:  192.9ms +/- 0.0%       1818.5ms +/- 0.1%
     8:  190.4ms +/- 0.0%       1816.6ms +/- 0.1%
    12:  190.6ms +/- 0.0%       1809.8ms +/- 0.1%
    16:  190.1ms +/- 0.0%       1807.4ms +/- 0.1%
    32:  190.3ms +/- 0.0%       1805.9ms +/- 0.1%  <--- best kraken
    64:  189.9ms +/- 0.0%       1806.8ms +/- 0.1%
   128:  189.7ms +/- 0.0%       1807.5ms +/- 0.1%  <--- best sunspider
   256:  190.1ms +/- 0.1%       1808.8ms +/- 0.1%
   512:  190.3ms +/- 0.0%       1806.1ms +/- 0.1%

Kraken improvements are mostly coming from attachment 657969 [details] [diff] [review] which provide similar results for larger values.  The reasons is explained with the increase of run time which flatten results of eager inlining.  Sunspider improvements are mostly coming from the current heuristic which prevent eager inlining if there is not enough call of the function to be inlined.  Alone, attachment 657969 [details] [diff] [review] is not performing well on sunspider because it was increasing the spectrum of inlinable functions and we can see that by pushing the ratio value to the highest numbers.

On top of 128 (master + attachment 657969 [details] [diff] [review] + heuristic), attachment 657469 [details] [diff] [review] regress sunspider by 0.2ms (189.7 -> 189.9) and improve kraken by 14.1ms (1807.5 -> 1793.4).

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Inline with type-checked arguments.

- Smoothly rebased with git without any issue.
- Follow renaming of IonBuilder::script field.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: Inline with type-checked arguments.

- Fix x86 compilation.
- Make defineAs work with any boxing instruction and prevent messing up with register allocations.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: Inline with type-checked arguments.

Delta:
 - Rebase.
 - Remove a few argc argument from IonBuilder. (as we now inline overflow)
 - Skip type barriers on overflow of arguments.

https://tbpl.mozilla.org/?tree=Try&rev=12cac6488768

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 699307 [details] [diff] [review]
Inline with type-checked arguments.

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

r=me w/ comments addressed

::: js/src/ion/IonBuilder.cpp
@@ +2856,5 @@
> +        if (excluded->target == calleeObs) {
> +            JS_ASSERT(callerObs->hasType(excluded->type));
> +
> +            if (excluded->type == types::Type::DoubleType() &&
> +                calleeObs->hasType(types::Type::Int32Type())) {

Per the IRC discussion with Brian, let's not add the double/int conversion code for now. It won't have much of an effect since outside Ion, the callee's types will be polluted anyway.

@@ +2935,5 @@
> +            types::StackTypeSet *argTypeSet = NULL;
> +            types::StackTypeSet *obsTypeSet = NULL;
> +            if (!i) {
> +                argTypeSet = this->oracle->getCallTarget(script(), argc, pc);
> +                obsTypeSet = oracle.thisTypeSet(calleeScript);

nit: Pull these two lines out of the loop, to avoid the one-off if.

@@ +2942,5 @@
> +                obsTypeSet = oracle.parameterTypeSet(calleeScript, i - 1);
> +            }
> +
> +            tmp = addTypeBarrier(tmp, argTypeSet, obsTypeSet, argsBarriers);
> +            if (!tmp)

addTypeBarrier is infallible.

::: js/src/ion/LOpcodes.h
@@ +99,5 @@
>      _(ValueToInt32)                 \
>      _(DoubleToInt32)                \
>      _(TruncateDToInt32)             \
> +    _(OptionalDToInt32V)            \
> +    _(OptionalVToInt32V)            \

(these can be removed)

::: js/src/ion/Lowering.cpp
@@ +1294,5 @@
>      }
>  }
>  
>  bool
> +LIRGenerator::visitOptionalToInt32(MOptionalToInt32 *ins)

(this can be removed)

::: js/src/ion/MIR.h
@@ +427,5 @@
> +#ifdef JS_NUNBOX32
> +    // This is used to prevent unboxing of instructions which are just wrapping
> +    // a value in a box to do some checking without changing the register
> +    // allocations.
> +    virtual bool isBoxing() const {

(this can be removed since MOptionalToInt32 goes away.)

@@ +1538,5 @@
> +
> +#ifdef JS_NUNBOX32
> +    bool isBoxing() const {
> +        return true;
> +    }

(this too.)

@@ +5805,5 @@
>      }
>  };
>  
> +// Prevent one type to flow in the next set of instructions. If this hypothesis
> +// fails it does not cause an invalidation but the monitored type should do it.

This comment is a bit confusing, how about something like...

// Guards that the incoming value does not have the specified Type.

@@ +5835,5 @@
> +    }
> +#endif
> +
> +    bool congruentTo(MDefinition * const &ins) const {
> +        return congruentIfOperandsEqual(ins);

This also needs to consider type_, but it should probably just return false.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 699307 [details] [diff] [review]
Inline with type-checked arguments.

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

r=me w/ comments addressed

::: js/src/ion/IonBuilder.cpp
@@ +2856,5 @@
> +        if (excluded->target == calleeObs) {
> +            JS_ASSERT(callerObs->hasType(excluded->type));
> +
> +            if (excluded->type == types::Type::DoubleType() &&
> +                calleeObs->hasType(types::Type::Int32Type())) {

Per the IRC discussion with Brian, let's not add the double/int conversion code for now. It won't have much of an effect since outside Ion, the callee's types will be polluted anyway.

@@ +2935,5 @@
> +            types::StackTypeSet *argTypeSet = NULL;
> +            types::StackTypeSet *obsTypeSet = NULL;
> +            if (!i) {
> +                argTypeSet = this->oracle->getCallTarget(script(), argc, pc);
> +                obsTypeSet = oracle.thisTypeSet(calleeScript);

nit: Pull these two lines out of the loop, to avoid the one-off if.

@@ +2942,5 @@
> +                obsTypeSet = oracle.parameterTypeSet(calleeScript, i - 1);
> +            }
> +
> +            tmp = addTypeBarrier(tmp, argTypeSet, obsTypeSet, argsBarriers);
> +            if (!tmp)

addTypeBarrier is infallible.

::: js/src/ion/LOpcodes.h
@@ +99,5 @@
>      _(ValueToInt32)                 \
>      _(DoubleToInt32)                \
>      _(TruncateDToInt32)             \
> +    _(OptionalDToInt32V)            \
> +    _(OptionalVToInt32V)            \

(these can be removed)

::: js/src/ion/Lowering.cpp
@@ +1294,5 @@
>      }
>  }
>  
>  bool
> +LIRGenerator::visitOptionalToInt32(MOptionalToInt32 *ins)

(this can be removed)

::: js/src/ion/MIR.h
@@ +427,5 @@
> +#ifdef JS_NUNBOX32
> +    // This is used to prevent unboxing of instructions which are just wrapping
> +    // a value in a box to do some checking without changing the register
> +    // allocations.
> +    virtual bool isBoxing() const {

(this can be removed since MOptionalToInt32 goes away.)

@@ +1538,5 @@
> +
> +#ifdef JS_NUNBOX32
> +    bool isBoxing() const {
> +        return true;
> +    }

(this too.)

@@ +5805,5 @@
>      }
>  };
>  
> +// Prevent one type to flow in the next set of instructions. If this hypothesis
> +// fails it does not cause an invalidation but the monitored type should do it.

This comment is a bit confusing, how about something like...

// Guards that the incoming value does not have the specified Type.

@@ +5835,5 @@
> +    }
> +#endif
> +
> +    bool congruentTo(MDefinition * const &ins) const {
> +        return congruentIfOperandsEqual(ins);

This also needs to consider type_, but it should probably just return false.

Comment 6

[Hannes Verschore [:h4writer]]

@nbp: just a heads-up, since bug 824473 the changes in IonBuilder.cpp will probably fail horribly. Sorry. Also 2-3 bugs are blocked by this bug, it would be handy to get this in sooner than later ;).

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to David Anderson [:dvander] from comment #5)
> ::: js/src/ion/IonBuilder.cpp
> @@ +2856,5 @@
> > +        if (excluded->target == calleeObs) {
> > +            JS_ASSERT(callerObs->hasType(excluded->type));
> > +
> > +            if (excluded->type == types::Type::DoubleType() &&
> > +                calleeObs->hasType(types::Type::Int32Type())) {
> 
> Per the IRC discussion with Brian, let's not add the double/int conversion
> code for now. It won't have much of an effect since outside Ion, the
> callee's types will be polluted anyway.

I will check that we don't bailout too much when we only check for the excluded type, and try what happen when we bail when converting to an Int.  If both solution are giving bad performances, I will report back here.  I think I tried one of them in the past, but I can't remember which one.  I just remember that I had to implement this extra path to avoid regressions.

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #7)
> (In reply to David Anderson [:dvander] from comment #5)
> > ::: js/src/ion/IonBuilder.cpp
> > @@ +2856,5 @@
> > > +        if (excluded->target == calleeObs) {
> > > +            JS_ASSERT(callerObs->hasType(excluded->type));
> > > +
> > > +            if (excluded->type == types::Type::DoubleType() &&
> > > +                calleeObs->hasType(types::Type::Int32Type())) {
> > 
> > Per the IRC discussion with Brian, let's not add the double/int conversion
> > code for now. It won't have much of an effect since outside Ion, the
> > callee's types will be polluted anyway.
> 
> I will check that we don't bailout too much when we only check for the
> excluded type, and try what happen when we bail when converting to an Int. 
> If both solution are giving bad performances, I will report back here.  I
> think I tried one of them in the past, but I can't remember which one.  I
> just remember that I had to implement this extra path to avoid regressions.

I checked on sunspider and octane. Doing always an exclude type regress a few benchmarks.  Namely PdfJS by ~3% and GameBoy by ~5%.

So, I am replacing the OptionToInt32 by a ToDouble followed by a ToInt32, which is slow and bailout on non-number inputs.

Comment 9

[Nicolas B. Pierron [:nbp]]

(In reply to David Anderson [:dvander] from comment #5)
> ::: js/src/ion/MIR.h
> @@ +427,5 @@
> > +#ifdef JS_NUNBOX32
> > +    // This is used to prevent unboxing of instructions which are just wrapping
> > +    // a value in a box to do some checking without changing the register
> > +    // allocations.
> > +    virtual bool isBoxing() const {
> 
> (this can be removed since MOptionalToInt32 goes away.)
> 
> @@ +1538,5 @@
> > +
> > +#ifdef JS_NUNBOX32
> > +    bool isBoxing() const {
> > +        return true;
> > +    }
> 
> (this too.)

This is needed by excludeType which use defineAs to reuse the guarded type which might come out of a TypeBarrier.  Without doing such a thing we would have taken a bad virtual register which is never defined, just by adding one to it, which might not work when the value is boxed.

Comment 10

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #6)
> @nbp: just a heads-up, since bug 824473 the changes in IonBuilder.cpp will
> probably fail horribly. Sorry. Also 2-3 bugs are blocked by this bug, it
> would be handy to get this in sooner than later ;).

The part which checks for excluded types is in inlinedScriptedCall which is only called from jsop_call.  So this should be fine.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

I don't understand why the defineAs/isBoxing stuff is needed - please re-r? if we plan on keeping it.

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Actually, given that the MExcludeType is a guard, why does it have any definition at all?

Comment 13

[Nicolas B. Pierron [:nbp]]

(In reply to David Anderson [:dvander] from comment #12)
> Actually, given that the MExcludeType is a guard, why does it have any
> definition at all?

I got inspiration from what we produce with MUnbox under pushTypeBarrier.  I am not sure to understand how we can move instructions around without having a guard on the data flow.  Can you en-light me on that?

Comment 14

[Nicolas B. Pierron [:nbp]]

Attachment: Inline with type-checked arguments.

dvander: ExcludeType need the isBoxing because, as GuardObject, it guards the data flow and make sure that the data cannot flow into the inlined function without being checked.

isBoxing is needed to avoid non-consecutive virtual registers boxing when we are trying to inferred tag's virtual register from the payload virtual register index.  This is used to reuse the payload while avoiding virtual register issues.

h4writer: I think this rebased patch should work fine as I am adding argsBarrier into all CallInfo which are likely to fall into the inlining case.
I was a bit surprised that we are not inlining JS getters yet, but this is another issue.

Comment 15

[Hannes Verschore [:h4writer]]

Comment on attachment 708675 [details] [diff] [review]
Inline with type-checked arguments.

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

Cool. I mostly looked to the IonBuilder changes. Think the hardest work is done, still I have some suggestions.
But I would like to see it again. Will probably be an auto r+ afterwards, but there are some bigger changes...

::: js/src/ion/IonBuilder.cpp
@@ +3060,5 @@
>  
>  bool
>  IonBuilder::jsop_call_inline(HandleFunction callee, CallInfo &callInfo, MBasicBlock *bottom,
>                               Vector<MDefinition *, 8, IonAllocPolicy> &retvalDefns)
> +// add types::TypeBarrier *argsBarriers into CallInfo.

As far as I can see your did this. This comment can get removed

@@ +3125,5 @@
> +        }
> +
> +        argv[i] = addTypeBarrier(current->pop(), argTypeSet, obsTypeSet, argsBarriers);
> +    }
> +    */

Remove comment, as this is now done in CallInfo ...

::: js/src/ion/IonBuilder.h
@@ +441,5 @@
>      MCall *makeCallHelper(HandleFunction target, CallInfo &callInfo,
>                            types::StackTypeSet *calleeTypes, bool cloneAtCallsite);
>      bool makeCallBarrier(HandleFunction target,  CallInfo &callInfo,
>                           types::StackTypeSet *calleeTypes, bool cloneAtCallsite);
>      bool makeCall(HandleFunction target, CallInfo &callInfo, 

1) While you're at it, remove this white-space.
2) on the makeCall* functions you can remove the "types::StackTypeSet *calleeTypes" argument. That equals callInfo.thisType(). Therefore it would be good to remove that argument from all calls and use callInfo.thisType().
3) While you're at it, could you move "cloneAtCallsite" also into callInfo?

@@ +522,5 @@
>      MInstruction *lazyArguments_;
>  };
>  
>  class CallInfo
>  {

I miss a hasCallType() function + add JS_ASSERT(callInfo->hasCallType); in inlineScriptedCall/inlineScriptedCalls/jsop_call_inline

@@ +600,5 @@
>  
>          return true;
>      }
>  
> +    bool initCallType(TypeOracle *oracle, HandleScript script, jsbytecode *pc) {

Because now we will probably need this information for all calls (atleast the thisType), maybe move this into init(current, pc) ?

::: js/src/ion/MIR.cpp
@@ +1556,5 @@
>          return input;
>  
>      if (input->type() == MIRType_Double && input->isConstant()) {
>          const Value &v = input->toConstant()->value();
> +        int32_t ret = ToInt32(v.toDouble());

Good catch

Comment 16

[Nicolas B. Pierron [:nbp]]

Comment on attachment 708675 [details] [diff] [review]
Inline with type-checked arguments.

https://tbpl.mozilla.org/?tree=Try&rev=c9f16168b2be

Comment 17

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #15)
> ::: js/src/ion/IonBuilder.h
> @@ +441,5 @@
> >      MCall *makeCallHelper(HandleFunction target, CallInfo &callInfo,
> >                            types::StackTypeSet *calleeTypes, bool cloneAtCallsite);
> >      bool makeCallBarrier(HandleFunction target,  CallInfo &callInfo,
> >                           types::StackTypeSet *calleeTypes, bool cloneAtCallsite);
> >      bool makeCall(HandleFunction target, CallInfo &callInfo, 
> 
> 2) on the makeCall* functions you can remove the "types::StackTypeSet
> *calleeTypes" argument. That equals callInfo.thisType(). Therefore it would
> be good to remove that argument from all calls and use callInfo.thisType().
> 3) While you're at it, could you move "cloneAtCallsite" also into callInfo?

I will add that into a separated patch as this one is becoming big and as these changes seems unrelated.

> @@ +522,5 @@
> >      MInstruction *lazyArguments_;
> >  };
> >  
> >  class CallInfo
> >  {
> 
> I miss a hasCallType() function + add JS_ASSERT(callInfo->hasCallType); in
> inlineScriptedCall/inlineScriptedCalls/jsop_call_inline

Good point, at least we would be able to check CallInfo which have not been initialized properly :)

> @@ +600,5 @@
> >  
> >          return true;
> >      }
> >  
> > +    bool initCallType(TypeOracle *oracle, HandleScript script, jsbytecode *pc) {
> 
> Because now we will probably need this information for all calls (atleast
> the thisType), maybe move this into init(current, pc) ?

I tried and looked carefully.  Adding such information to the init function is a bit tedious as all calls are not as easy to map and only a few calls are inlined.  For example, properties getters and setters are not yet inlined because of a snapshot issue, but fetching the type from a getprop involve a totally different TypeOracle functions, even if these are still popped arguments.

This is the reason why it took me a bit longer than expected to merge this :(, and why I made another init function for the call types.

Comment 18

[Hannes Verschore [:h4writer]]

Ah right, in that case, can you make "init()" to initialize the "thisType" property only and initCallType all other information needed for inlining?

Comment 19

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #18)
> Ah right, in that case, can you make "init()" to initialize the "thisType"
> property only and initCallType all other information needed for inlining?

I don't even think I can factor thisType initialization in the init function, just look at initFunApplyArguments & initCallType.  In addition, I don't see the point of having such information if it is unused.

Comment 20

[Nicolas B. Pierron [:nbp]]

Attachment: Inline with type-checked arguments.

Delta:
- (h4writer nit) Remove temp comments
- (h4writer nit) Add hasCallType predicate.
- Use excluded types in InlinedScriptCall. (copied from jsop_call_inline)

Comment 21

[Hannes Verschore [:h4writer]]

Comment on attachment 709001 [details] [diff] [review]
Inline with type-checked arguments.

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

1) In TypeOracle.cpp:canInlineCall you don't remove the "caller->analysis()->typeBarriers(cx, pc)" check. Therefore we never inline functions that need the barrier ...

2) How does this all work with FunApply?

The argument numbers are not correct: function test(a,b,c) { foo.apply({}, [1,2,3]) }

// callInfo.initFunApplyArguments
// info().nargs()
// => That is the totally wrong. That's the number of arguments the function that has fun.apply(); in it.
// => That's [a,b,c] in the example

// callInfo.argv()
// callee->nargs
// => That's [{}, 1, 2, 3]

// inlineScriptedCall
for (int32_t i = 1; i <= callee->nargs; i++)
    addTypeBarrier(i, callInfo, oracle.parameterTypeSet(calleeScript, i - 1));
// => That's [{}, 1, 2, 3]

I have no idea if you need the original arguments [foo, {}, [1,2,3]] or the inlined arguments [{}, 1, 2, 3]. Can you elaborate on this?

I would be in favor to NOT inline funapply calls that need "caller->analysis()->typeBarriers(cx, pc)" for now,
because of this. Would simplify this patch ... Let me know what you think

::: js/src/ion/IonBuilder.cpp
@@ +2968,5 @@
> +        callerObs = callinfo.thisType();
> +    } else {
> +        ins = callinfo.getArg(i - 1);
> +        callerObs = callinfo.getArgType(i - 1);
> +    }

ins = (i == 0) ? callinfo.thisArg() : callinfo.getArg(i - 1);
callerObs = (i == 0) ? callinfo.thisType() : callinfo.getArgType(i - 1);

I think that would be cleaner.

@@ +3069,5 @@
>  IonBuilder::jsop_call_inline(HandleFunction callee, CallInfo &callInfo, MBasicBlock *bottom,
>                               Vector<MDefinition *, 8, IonAllocPolicy> &retvalDefns)
>  {
>      AssertCanGC();
> +    JS_ASSERT(callInfo.hasCallType());

Add assert that we don't get a JSOP_FUNAPPLY here ever.

::: js/src/ion/IonBuilder.h
@@ +615,5 @@
> +
> +    bool initFunApplyArguments(TypeOracle *oracle, HandleScript script, jsbytecode *pc, uint32_t nargs) {
> +        argsBarriers_ = oracle->callArgsBarrier(script, pc);
> +        thisType_ = oracle->getCallArg(script, 2, 0, pc);
> +        if (!argsType_.reserve(argc()))

s/argc()/nargs/

Comment 22

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #21)
> 2) How does this all work with FunApply?
> 
> The argument numbers are not correct: function test(a,b,c) { foo.apply({},
> [1,2,3]) }
> 
> // callInfo.initFunApplyArguments
> // info().nargs()
> // => That is the totally wrong. That's the number of arguments the function
> that has fun.apply(); in it.
> // => That's [a,b,c] in the example
> 
> // callInfo.argv()
> // callee->nargs
> // => That's [{}, 1, 2, 3]

=> That's [1,2,3]
for both, as this is put in its own slot.

> // inlineScriptedCall
> for (int32_t i = 1; i <= callee->nargs; i++)
>     addTypeBarrier(i, callInfo, oracle.parameterTypeSet(calleeScript, i -
> 1));
> // => That's [{}, 1, 2, 3]
> 
> I have no idea if you need the original arguments [foo, {}, [1,2,3]] or the
> inlined arguments [{}, 1, 2, 3]. Can you elaborate on this?
> 
> I would be in favor to NOT inline funapply calls that need
> "caller->analysis()->typeBarriers(cx, pc)" for now,
> because of this. Would simplify this patch ... Let me know what you think

I agree, otherwise we the solution involved in this patch was to take the JSFunction->nargs which represent the number of formal arguments.  And as we disable SetArg with lazy arguments, we can safely re-use the type of the argument of the caller.  I am not sure to understand how we should handle checks on arguments which are not part of the caller but expected by the callee.

> ::: js/src/ion/IonBuilder.cpp
> @@ +2968,5 @@
> > +        callerObs = callinfo.thisType();
> > +    } else {
> > +        ins = callinfo.getArg(i - 1);
> > +        callerObs = callinfo.getArgType(i - 1);
> > +    }
> 
> ins = (i == 0) ? callinfo.thisArg() : callinfo.getArg(i - 1);
> callerObs = (i == 0) ? callinfo.thisType() : callinfo.getArgType(i - 1);
> 
> I think that would be cleaner.

This is a question of style, and I find the if-else more readable.

> @@ +3069,5 @@
> >  IonBuilder::jsop_call_inline(HandleFunction callee, CallInfo &callInfo, MBasicBlock *bottom,
> >                               Vector<MDefinition *, 8, IonAllocPolicy> &retvalDefns)
> >  {
> >      AssertCanGC();
> > +    JS_ASSERT(callInfo.hasCallType());
> 
> Add assert that we don't get a JSOP_FUNAPPLY here ever.

We already do so below.
 
> ::: js/src/ion/IonBuilder.h
> @@ +615,5 @@
> > +
> > +    bool initFunApplyArguments(TypeOracle *oracle, HandleScript script, jsbytecode *pc, uint32_t nargs) {
> > +        argsBarriers_ = oracle->callArgsBarrier(script, pc);
> > +        thisType_ = oracle->getCallArg(script, 2, 0, pc);
> > +        if (!argsType_.reserve(argc()))
> 
> s/argc()/nargs/

1/ Agreed. 2/ Does not matter much. 3/ I removed it ;)

Comment 23

[Nicolas B. Pierron [:nbp]]

Attachment: Inline with type-checked arguments.

Delta:
 - Remove support for fun.apply inlining.  In the mean time I added an assertion to make sure TI does not give any argument barrier when we use fun.apply.  If it segv, we should add a test case and guard against inlining in such cases.

Comment 24

[Nicolas B. Pierron [:nbp]]

Comment on attachment 709040 [details] [diff] [review]
Inline with type-checked arguments.

As h4writer commented on IRC, this patch does not remove the check which has been moved from IonBuilder into the in the Type Oracle (merge issue) and cause the current patch to be a no-op.

This makes me wonder that I should probably re-evaluate the cost of MOptionToInt32, after making this patch.

In the mean time, fixing the type oracle with this patch reveal an other error that I will investigate.

Comment 25

[Nicolas B. Pierron [:nbp]]

Attachment: Inline with type-checked arguments.

Delta:
1/ Fix the oracle (as mentioned before)
2/ Fix iteration on arguments (iterate on the min of actual arguments / target->nargs) for generating excluded types.

I fixed an issue (2) which was related to the use of this optimization, so I am confident that this optimization is running with this patch and with the number of eager compilation of Ion Monkey.

Comment 26

[Hannes Verschore [:h4writer]]

Comment on attachment 710121 [details] [diff] [review]
Inline with type-checked arguments.

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

The upper stages look good. Dvander: in previous reviews you wanted isBoxing gone. This is still in the patch (and used). I'll let you review the lowering/codegenerator stuff ;).

Comment 27

[Nicolas B. Pierron [:nbp]]

Attachment: Inline with type-checked arguments.

Delta:
 - Rebase
 - Move ExcludeType as a pure guard.
 - Remove isBoxing thing.
 - Move the JSFunction constant to the parent block such as the resume point can capture a correct state.

Comment 28

[Nicolas B. Pierron [:nbp]]

Comment on attachment 716270 [details] [diff] [review]
Inline with type-checked arguments.

https://tbpl.mozilla.org/?tree=Try&rev=3170205be236

Comment 29

[Nicolas B. Pierron [:nbp]]

Comment on attachment 710121 [details] [diff] [review]
Inline with type-checked arguments.

This patch is obsoleted by the new one.  Keep h4writer review+ for the new patch.

Comment 30

[Nicolas B. Pierron [:nbp]]

https://tbpl.mozilla.org/?tree=Try&rev=3170205be236

Comment 31

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 716270 [details] [diff] [review]
Inline with type-checked arguments.

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

::: js/src/ion/IonBuilder.h
@@ +642,5 @@
> +        thisType_ = oracle->getCallArg(script, 2, 0, pc);
> +        if (!argsType_.reserve(nargs))
> +            return false;
> +        for (uint32_t i = 0; i < nargs; i++) {
> +            if (!argsType_.append(oracle->parameterTypeSet(script, i)))

are these appends infallible after reserve?

Comment 32

[Nicolas B. Pierron [:nbp]]

(In reply to David Anderson [:dvander] from comment #31)
> Comment on attachment 716270 [details] [diff] [review]
> Inline with type-checked arguments.
> 
> Review of attachment 716270 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/IonBuilder.h
> @@ +642,5 @@
> > +        thisType_ = oracle->getCallArg(script, 2, 0, pc);
> > +        if (!argsType_.reserve(nargs))
> > +            return false;
> > +        for (uint32_t i = 0; i < nargs; i++) {
> > +            if (!argsType_.append(oracle->parameterTypeSet(script, i)))
> 
> are these appends infallible after reserve?

Yes they are, but I feel bad of not having a dead branch on a boolean result.  I guess can use a DebugOnly If I am not pulling too many headers.

Comment 33

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/437c955ff06d

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/437c955ff06d