for review (XXL): 6655638 method handles for invokedynamic

[Vladimir Kozlov]

John,

Since delayed_value implementation can be safely separated
from the rest of your changes can you push it separately?
Here is review for it. In general it is good.

-------------------
Why you did not put class RegisterConstant into src/share/vm/asm/assembler.hpp?
You can leave a comment where to look for it.
-------------------
MacroAssembler::delayed_value() uses RegisterConstant constructors
implicitly. Can you change it to explicit usage?
-------------------
assembler_x86.cpp/MacroAssembler::load_sized_value()

And you asked Christian don't use high/low words :) :

6251   // For case 8, caller is responsible for manually loading
6252   // the high word into another register.

Also why you are using movptr() in the corresponding part in
MethodHandles::generate_method_handle_stub() ?:

  494         __ load_sized_value(rbx_temp, prim_value_addr,
  495                             type2aelembytes(arg_type), is_signed_subword_type(arg_type));
  496         __ movptr(Address(rax_argslot, 0), rbx_temp);
  497 #ifndef _LP64
  498         if (arg_slots == 2) {
  499           __ movl(rbx_temp, prim_value_addr.plus_disp(wordSize));
  500           __ movptr(Address(rax_argslot, Interpreter::stackElementSize()), rbx_temp);
  501         }
  502 #endif //_LP64

And may be better use load_signed_short/load_unsigned_short names:

6261   case ~2:  load_signed_word(   dst, src ); break;
6262   case  2:  load_unsigned_word( dst, src ); break;
-------------------
assembler.cpp:

We talked about to comment why DC_LIMIT = 20.
-------------------
assembler.hpp:

I think, instead of "// Use sparingly" it may be better to explain
that is mostly used for java classes non-static field offsets
(currently only for MethodHandle) when we need to generated assembler
code before the class is loaded and field offsets are not known.
Also that is why the value can't be 0 since an object has markword
at 0 offset.

  285   // The value zero (NULL) stands instead of a constant which is still uncomputed.
  286   // Use sparingly.
-------------------
Why the next method does not return intptr_t* also?

  static address*  delayed_value_addr(address(*constant_fn)());
-------------------
The variable last_WrongMethodType_caller is declared but is not used:
src/cpu/sparc/vm/interpreter_sparc.cpp
-------------------
On x64 in 2 files in TemplateTable::prepare_invoke()
why you changed movptr(recv  to  movl(recv / movq(recv ?
Did you fix a problem? It should be separate bug in that case?
-------------------

Thanks,
Vladimir


John Rose wrote:
> This is part of the JVM side of the JSR 292 Reference Implementation.  
> It should have no effect on application execution, unless one of the new 
> flags is turned on (chiefly -XX:+MethodHandleSupport).
> 
> Present limitations:
>  - It works only on x86/32.  (No sparc, compressed oops, cpp interpreter.)
>  - There are no compiler optimizations.
>  - It is young code.  There are bugs.  But only when a new flag is 
> turned on.
> 
> This review is for contents of meth.patch, to be pushed from mlvm to 
> http://hg.openjdk.java.net/jdk7/hotspot-comp-gate/hotspot .
> 
> Here is the webrev for this review:
>   http://webrev.invokedynamic.info/jrose/6655638.meth/
> 
> Here is the mlvm patch where the code currently lives:
>   http://hg.openjdk.java.net/mlvm/mlvm/hotspot/raw-file/tip/meth.patch
> 
> An earlier version of these changes pass JPRT; it is expected that at 
> most minor tweaks will be needed to push it through again.
> 
> Even though they are large, the changes should also pass a simple visual 
> inspection, since all substantially new control paths are guarded by 
> tests of the new flags.
> 
> Please give it a look.
> 
> -- John
> 
> P.S.  Here are some relevant conversations about method handles and 
> invokedynamic:
>   http://mail.openjdk.java.net/pipermail/mlvm-dev/2009-January/000321.html
>   http://blogs.sun.com/jrose/entry/international_invokedynamic_day
>   http://blogs.sun.com/dannycoward/entry/firing_up_the_engines_for
>   
> http://groups.google.com/group/jvm-languages/browse_thread/thread/a4b8a616eb987ca8 
> 
> 
> P.P.S. The proposed JDK changes are independent.  At present, you can 
> find them here, in patch and webrev formats:
>   http://hg.openjdk.java.net/mlvm/mlvm/hotspot/raw-file/tip/meth.proj.patch
>   http://webrev.invokedynamic.info/jrose/6655638.meth.proj/
>