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

[Christian Thalinger]

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