review for 7071307: MethodHandle bimorphic inlining should consider the frequency

[Christian Thalinger]

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