Request for reviews (L): 6939203: JSR 292 needs method handle constants
John Rose
John.Rose at Sun.COM
Tue Apr 6 14:44:19 PDT 2010
On Apr 6, 2010, at 8:07 AM, Christian Thalinger wrote:
> On Tue, 2010-03-30 at 04:24 -0700, John Rose wrote:
>> 6939203: JSR 292 needs method handle constants
>> Summary: Add new CP types CONSTANT_MethodHandle, CONSTANT_MethodType; extend 'ldc' bytecode.
>> http://cr.openjdk.java.net/~jrose/6939203/hs-webrev.00
>
> src/share/vm/interpreter/bytecode.hpp:
>
> 404 inline friend Bytecode_loadconstant* Bytecode_loadconstant_at(address bcp, methodHandle method);
> 405 inline friend Bytecode_loadconstant* Bytecode_loadconstant_at(methodHandle method, int bci);
>
> Why are the arguments swapped? int and address should be different
> types.
The (bcp, optional method) pattern mimics Bytecodes::code_at.
The (method, bci) pattern mimics a methodOop accessor.
The patterns don't mix well. I'll try to boil this down to fewer overloadings.
> src/share/vm/interpreter/rewriter.cpp:
>
> 142 Bytecodes::Code fastc = Bytecodes::_nop;
> 143 if (tag.is_method_handle() || tag.is_method_type()) {
> 144 fastc = (is_wide
> 145 ? Bytecodes::_fast_aldc_w
> 146 : Bytecodes::_fast_aldc);
> 147 }
> 148 if (fastc != Bytecodes::_nop) {
>
> Why do you have a second if here?
No particular reason, other than leaving open an option to extend the aldc treatment to classes or other (future) constant types. I'll merge the two blocks.
> src/share/vm/prims/jvm.h:
>
> + JVM_CONSTANT_MethodHandle = 15, // JSR 292
> + JVM_CONSTANT_MethodType = 16 // JSR 292
>
> Is there a particular reason I don't see why this is 15 and 16?
Yes; I think the modules project is going to use the code point 13 and I wanted to leave a little extra room after it (14) in case they suddenly invent a second CP tag. This will all look very funny in ten years.
>> Please review this small JDK change along with it:
>> http://cr.openjdk.java.net/~jrose/6939203/jdk-webrev.00
>
> Looks good.
Thanks!
-- John
More information about the hotspot-compiler-dev
mailing list