review for 7071307: MethodHandle bimorphic inlining should consider the frequency

[Christian Thalinger]

On Aug 26, 2011, at 10:47 PM, Tom Rodriguez wrote:

> I needed an is_empty() test in addition to the method_data() != NULL.
> 
>    if (_caller_jvms != NULL && _caller_jvms->method() != NULL &&
>        _caller_jvms->method()->method_data() != NULL &&
>        !_caller_jvms->method()->method_data()->is_empty()) {
>      ciMethodData* mdo = _caller_jvms->method()->method_data();
>      ciProfileData* mha_profile = mdo->bci_to_data(_caller_jvms->bci());
>      assert(mha_profile, "must exist");
>      CounterData* cd = mha_profile->as_CounterData();
>      call_site_count = cd->count();
>    } else {
>      call_site_count = invoke_count;  // use the same value                                                                                          
>    }

Looks good.

> 
> I also hit another unrelated assertion when running the test where he had optimized away all the invokedynamics, so Compile::has_method_handle_invokes was true but we never actually emitted any.  So we failed this assert in nmethod.cpp:
> 
>  assert(has_method_handle_invokes() == (_deoptimize_mh_offset != -1), "must have deopt mh handler");
> 
> The fix is to remove the set_has_method_handle_invokes call in callGenerator.cpp and set them when they are matched.
> 
> diff -r ac8738449b6f src/share/vm/opto/matcher.cpp                                                                                                    
> --- a/src/share/vm/opto/matcher.cpp                                                                                                                  
> +++ b/src/share/vm/opto/matcher.cpp                                                                                                                  
> @@ -1106,6 +1106,9 @@
>       mcall_java->_optimized_virtual = call_java->is_optimized_virtual();                                                                            
>       is_method_handle_invoke = call_java->is_method_handle_invoke();                                                                                
>       mcall_java->_method_handle_invoke = is_method_handle_invoke;                                                                                  
> +      if (is_method_handle_invoke) {                                                                                                                
> +        C->set_has_method_handle_invokes(true);                                                                                                      
> +      }                                                                                                                                              
>       if( mcall_java->is_MachCallStaticJava() )                                                                                                      
>         mcall_java->as_MachCallStaticJava()->_name =                                                                                                
>          call_java->as_CallStaticJava()->_name;

Ahh, good catch.

> 
> There's some crazy deep inlining in that smalltalk test case.  I think there must be some sort of bug with it.  The PrintInlining output wraps on my screen several times.  I'm looking at it.

I haven't printed the inlining tree yet.  I will try...

-- Christian

> 
> tom
> 
> On Aug 26, 2011, at 4:16 AM, Christian Thalinger wrote:
> 
>> I just applied this patch to test the rtalk implementation and I hit an assert:
>> 
>> Internal Error at bytecodeInfo.cpp:152, pid=10351, tid=11
>> assert(mha_profile) failed: must exist
>> 
>> Some context:
>> 
>> (dbx) p _caller_jvms->method()->print()
>> <ciMethod name=invoke holder=rtPbc/r68 signature=(Lri/core/rtalk/RtObject;Lri/core/rtalk/RtObject;Lri/core/rtalk/RtObject;)Lri/core/rtalk/RtObject; loaded=true flags=public,static ident=904 PERM address=0x8d6ab58>_caller_jvms->method()->print() = (void)
>> (dbx) p _caller_jvms->bci()             
>> _caller_jvms->bci() = 7
>> (dbx) p _caller_jvms->method()->print_codes()
>> 0 aload_2
>> 1 astore_3
>> 2 aload_1
>> 3 fast_aload_0
>> 4 aload_2
>> 5 astore_3
>> 6 aload_3
>> 7 invokedynamic secondary cache[4] of CP[2] missing bias?
>> 0   bci: 7    CounterData         count(16900)
>> 12 astore_3
>> 13 aload_3
>> 14 invokedynamic secondary cache[5] of CP[3] missing bias?
>> 8   bci: 14   CounterData         count(16900)
>> 19 astore_3
>> 20 aload_3
>> 21 areturn
>> _caller_jvms->method()->print_codes() = (void)
>> (dbx) p mdo->print()
>> --- Extra data:
>> mdo->print() = (void)
>> (dbx) 
>> 
>> -- Christian
>> 
>> On Aug 24, 2011, at 11:12 PM, Tom Rodriguez wrote:
>> 
>>> 
>>> On Aug 24, 2011, at 6:12 AM, Christian Thalinger wrote:
>>> 
>>>> 
>>>> On Aug 24, 2011, at 1:44 AM, Tom Rodriguez wrote:
>>>> 
>>>>> This is a re-review since I added per method handle GWT profiling.
>>>>> 
>>>>> http://cr.openjdk.java.net/~never/7071307
>>>>> 312 lines changed: 270 ins; 15 del; 27 mod; 22101 unchg
>>>> 
>>>> src/share/vm/prims/methodHandleWalk.cpp:
>>>> 
>>>> MethodHandleCompiler::fetch_counts:
>>>> 
>>>> +   int count1 = -1, count2 = -1;
>>>> ...
>>>> +   int total = count1 + count2;
>>>> +   if (count1 != -1 && count2 != -2 && total != 0) {
>>>> 
>>>> Why -2?
>>> 
>>> Just a typo.  It's fixed.
>>> 
>>>> 
>>>> +   int          _taken_count;
>>>> +   int          _not_taken_count;
>>>> 
>>>> Does taken refer to target and not_taken to fallback in the GWT?
>>> 
>>> They refer to the bytecode and the vmcounts collected.  I think they are actually reversed from what selectAlternative generates but as long as they agree with the bytecodes generated I don't think it matters.  I verified empirically that the counts match the execution and feed into the frequency in the proper fashion.
>>> 
>>>> 
>>>> MethodHandleCompiler::make_invoke:
>>>> 
>>>> Can you use emit_bc instead of _bytecode.push where possible so we have at least a little sanity checking?
>>> 
>>> I added support for ifeq and added update_branch_dest to correct the offsets.  I only added support for ifeq for now.
>>> 
>>>> 
>>>> +     bool found_sel = false;
>>>> 
>>>> Can you rename that to maybe found_selectAlternative?
>>> 
>>> Yup.
>>> 
>>>> 
>>>> 
>>>> src/share/vm/ci/ciMethodHandle.cpp:
>>>> 
>>>> That print_chain is very helpful.  Thanks for that.
>>>> 
>>>> 
>>>> src/share/vm/classfile/javaClasses.cpp:
>>>> 
>>>> + int java_lang_invoke_CountingMethodHandle::vmcount(oop mh) {
>>>> +   assert(is_instance(mh), "DMH only");
>>>> +   return mh->int_field(_vmcount_offset);
>>>> + }
>>>> + 
>>>> + void java_lang_invoke_CountingMethodHandle::set_vmcount(oop mh, int count) {
>>>> +   assert(is_instance(mh), "DMH only");
>>>> +   mh->int_field_put(_vmcount_offset, count);
>>>> + }
>>>> 
>>>> I think the assert message is a copy-paste bug.
>>> 
>>> Fixed.
>>> 
>>>> 
>>>> Otherwise looks good.
>>> 
>>> Thanks!
>>> 
>>> tom
>>> 
>>>> 
>>>>> 
>>>>> 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.
>>>>> 
>>>>> Additionally I added support for per method handle profile collection
>>>>> since this was required to get good results for more complex programs.
>>>>> This requires the fix for 7082631 on the JDK side.
>>>>> http://cr.openjdk.java.net/~never/7082631
>>>> 
>>>> The JDK changes look good.
>>>> 
>>>> -- Christian
>>>> 
>>>>> 
>>>>> 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.
>>>>> 
>>>> 
>>> 
>> 
>