review request (XXXL): 7023639: JSR 292 method handle invocation needs a fast path for compiled code

[Vladimir Kozlov]

Nice.

Vladimir

Christian Thalinger wrote:
> We forgot to remove the Ricochet Frame code from the sA:
> 
> http://cr.openjdk.java.net/~twisti/7023639/
> 
> -- Chris
> 
> On Jul 19, 2012, at 11:28 AM, Christian Thalinger wrote:
> 
>> JDK testing found a small bug:
>>
>> diff --git a/src/share/vm/oops/cpCacheOop.cpp b/src/share/vm/oops/cpCacheOop.cpp
>> --- a/src/share/vm/oops/cpCacheOop.cpp
>> +++ b/src/share/vm/oops/cpCacheOop.cpp
>> @@ -486,7 +486,8 @@
>>     // virtual and final so _f2 contains method ptr instead of vtable index
>>     if (f2_as_vfinal_method() == old_method) {
>>       // match old_method so need an update
>> -      set_f2_as_vfinal_method(new_method);
>> +      // NOTE: can't use set_f2_as_vfinal_method as it asserts on different values
>> +      _f2 = (intptr_t)new_method;
>>       if (RC_TRACE_IN_RANGE(0x00100000, 0x00400000)) {
>>         if (!(*trace_name_printed)) {
>>           // RC_TRACE_MESG macro has an embedded ResourceMark
>>
>> I will integrate this one.
>>
>> -- Chris
>>
>> On Jul 18, 2012, at 7:10 PM, John Rose wrote:
>>
>>> On Jul 17, 2012, at 4:04 PM, Christian Thalinger wrote:
>>>
>>>>> I see in several files next code pattern. Should we call throw_IncompatibleClassChangeError() as we do in other places?:
>>>>>
>>>>> +  if (!EnableInvokeDynamic) {
>>>>> +    // rewriter does not generate this bytecode
>>>>> +    __ should_not_reach_here();
>>>>> +    return;
>>>>> +  }
>>>> Hmm.  This really should not happen and EnableInvokeDynamic is on by default anyway.  I doubt someone turns it off.
>>> It's now a diagnostic switch.  It can be used to verify that an app. is not using 292, which is occasionally helpful.
>>>
>>>>> constantPoolOop.cpp:
>>>>> Why not use guarantee() for bad operants?
>>>> Not sure.  John?
>>> The code in that file uses assert; I think I'm following the existing practice there.  The CP code operates after initial verification passes, so assert is suitable, I think.
>>>
>>>>> Why you need separate scopes in resolve_bootstrap_specifier_at_impl()?
>>>> To keep oops from being visible.  Might result in bad crashes.
>>> Yes.  An alternative is to have the x_oop variables visible at top level, but to put in DEBUG_ONLY(x_oop = NULL).  Having the temporary scoped seems cleaner to me, but maybe my sense of style is off here.
>>>
>>>> I will refresh the review patch in mlvm.  Thank you!
>>> Yes, thanks!
>>>
>>> — John
>>> _______________________________________________
>>> mlvm-dev mailing list
>>> mlvm-dev at openjdk.java.net
>>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
>> _______________________________________________
>> mlvm-dev mailing list
>> mlvm-dev at openjdk.java.net
>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
>