review for 7071307: MethodHandle bimorphic inlining should consider the frequency

[Christian Thalinger]

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?

+   int          _taken_count;
+   int          _not_taken_count;

Does taken refer to target and not_taken to fallback in the GWT?

MethodHandleCompiler::make_invoke:

Can you use emit_bc instead of _bytecode.push where possible so we have at least a little sanity checking?

+     bool found_sel = false;

Can you rename that to maybe found_selectAlternative?


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.

Otherwise looks good.

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