review for 7071307: MethodHandle bimorphic inlining should consider the frequency
Tom Rodriguez
tom.rodriguez at oracle.com
Fri Aug 26 13:47:47 PDT 2011
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
}
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;
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.
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.
>>>>
>>>
>>
>
More information about the hotspot-compiler-dev
mailing list