review for 7071307: MethodHandle bimorphic inlining should consider the frequency

[Tom Rodriguez]

On Jul 29, 2011, at 11:26 AM, Vladimir Kozlov wrote:

> I just realized that your are generating diamond code and not bimorphic which has third failure path. And since you generate diamond it does not matter what probability of in(1) path. So the code you suggested will work. Sorry for the noise.

I was just replying with that explanation.  ;)  Thanks.

tom

> 
> Thanks,
> Vladimir
> 
> Vladimir Kozlov wrote:
>> Tom,
>> First, question: mh parameter passed to PredictedDynamicCallGenerator() is taking from method_handle->in(1) path, can in(2) has different mh?
>> Next, cg1 should have higher prob to generated it first in the code (it is passed as if_hit into PredictedDynamicCallGenerator()). I think you need to do it like this:
>> +     float prob = PROB_FAIR;
>> +     int if_hit = 1;
>> +     Node* meth_region = method_handle->in(0);
>> +     if (meth_region->is_Region() &&
>> +         meth_region->in(1)->is_Proj() && meth_region->in(2)->is_Proj() &&
>> +         meth_region->in(1)->in(0) == meth_region->in(2)->in(0) &&
>> +         meth_region->in(1)->in(0)->is_If()) {
>> +       // If diamond, so grab the probability of the test to drive the inlining below
>> +       prob = meth_region->in(1)->in(0)->as_If()->_prob;
>> +       if (meth_region->in(1)->is_IfFalse()) if_hit = 2;
>> +     }
>> +
>>      // selectAlternative idiom merging two constant MethodHandles.
>>      // Generate a guard so that each can be inlined.  We might want to
>>      // do more inputs at later point but this gets the most common
>>      // case.
>> +     if (prob < PROB_FAIR) {
>> +       // cg1 should have higher prob to generated it first in the code.
>> +       if_hit = 3-if_hit;
>> +       prob = 1.0 - prob;
>> +     }
>> +     CallGenerator* cg1 = for_method_handle_inline(method_handle->in(if_hit), jvms, caller, callee, profile.rescale(prob));
>> +     CallGenerator* cg2 = for_method_handle_inline(method_handle->in(3-if_hit), jvms, caller, callee, profile.rescale(1.0 - prob));
>> +     if (cg1 != NULL && cg2 != NULL) {
>> Thanks,
>> Vladimir
>> Tom Rodriguez wrote:
>>> On Jul 28, 2011, at 6:03 PM, Vladimir Kozlov wrote:
>>> 
>>>> Tom,
>>>> 
>>>> I think you need to check which path has IfTrue project to apply probability from IfNode.
>>> 
>>> I originally had code to do that but I convinced myself it wasn't needed though now I can't imagine what I was thinking.  Currently these if's are all structured such that they don't need to be swapped but there's no reason they couldn't be.  I've updated the webrev.
>>> 
>>> tom
>>> 
>>>> Vladimir
>>>> 
>>>> Tom Rodriguez wrote:
>>>>> http://cr.openjdk.java.net/~never/7071307
>>>>> 46 lines changed: 27 ins; 6 del; 13 mod; 3568 unchg
>>>>> 7071307: MethodHandle bimorphic inlining should consider the frequency
>>>>> Reviewed-by:
>>>>> The fix for 7050554 added a bimorphic inline path but didn't take into
>>>>> account the frequency of the guarding test.  This ends up treating
>>>>> both sides of the if as equally frequent which can lead to over
>>>>> inlining and overflowing the method inlining limits.  The fix is to
>>>>> grab the frequency from the If and apply that to the branches.  This
>>>>> addresses a major source of overinlining that can result in bad
>>>>> performance with JSR 292.  We may do a later extension to this to
>>>>> actually do per call chain profiling of selectAlternative but that's a
>>>>> more complicated fix.
>>>>> I also fixed a problem with the ideal graph printer where debug_orig
>>>>> printing would go into an infinite loop.
>>>>> Tested with jruby and vm.mlvm tests.
>>>