review for 7071307: MethodHandle bimorphic inlining should consider the frequency

[Tom Rodriguez]

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