for review (XXL): 6655638 method handles for invokedynamic

Tue Mar 31 13:08:43 PDT 2009

Most of my comments are below mostly about style and naming and could  
be addressed in later putbacks since this obviously isn't done.

MethodEntry is too general of a name.  Maybe you can come up with a  
better one.

CHECK_UNHANDLED_OOPS might be worth running at some point.

methodHandles.hpp:

I'm not a big fan of formatting like this:

   if (mh.is_null() || target.is_null() || ! 
java_dyn_MethodHandle::is_instance(target()))
     { throw_InternalError(CHECK); }

Could you format this and others normally?  At most it saves one  
line.  Also the throw_* functions seem a bit excessive.  It also  
defeats the fact that the THROW macro records the line number of the  
throw point which can be handy for debugging.  Do you distrust the  
THROW macro?  Actually, with a smart enough compiler they could lead  
to unreachable code warnings since you're effectively writing this:

{ Exceptions::_throw_msg(THREAD_AND_LOCATION, name, NULL); return;  }
{ report_should_not_reach_here  (__FILE__, __LINE__); BREAKPOINT; }

A lot of the methods take oop arguments but then immediately convert  
them into handles and have extra logic where the oop argument is  
nulled out for safety.  Some even take a mix of handles and oops.  Why  
not just use handles uniformly?

adapter_conversion could use some asserts validating the encoding.

When writing flags into the MembarName from access_flags().as_short()  
shouldn't you be masking them with JVM_RECOGNIZED_FIELD_MODIFIERS and  
JVM_RECOGNIZED_METHOD_MODIFIERS as appropriate?

There are big chunks of VerifyMethodHandles code in the moddle of  
other logic and it might be nice if these were broken out so that main  
code would be more readable.  I know in some cases that might be  
difficult but init_AdapterMethodHandle in particular has a chunk that  
might be movable.

Could you make the natives entry points to make it clear that even  
though they are JVM_ENTRY they aren't actually JVM_ entry points?

methodHandles_x86.cpp:

The padding code in MethodEntry::start_compiled_entry could use a  
comment explaining that it's reserving space in the beginning for the  
Data field.  The space before this trick feels a little too clever but  
I'm not totally against it.

Don't do the (int32_t)NULL_WORD cast here:

     __ cmpptr(arg_slots.as_register(), (int32_t) NULL_WORD);

In the add_arg_slots code shouldn't there be an assert in the if  
(TaggedStackInterpreter) piece that arg_slots is not a register since  
constant_arg_slots won't have a valid value in that case?

In trace_method_handle_stub the format should be:

   printf("MH %s %p %p "INTX_FORMAT"\n", adaptername, mh, entry_sp,  
entry_sp - saved_sp);

since entry_sp - saved_sp is an intptr_t.  You might use PTR_FORMAT  
instead of %p since %p isn't consistently defined.

Shouldn't these be movptr?

       __ movl(rax_klass, Address(rcx_recv,  
oopDesc::klass_offset_in_bytes()));
       __ movl(rdx_temp, Address(rdx_temp,  
oopDesc::klass_offset_in_bytes()));

Are you going to delete any #if 0 pieces still lying around?

What controls which values of the switch are valid in  
generate_method_handle_stub?  There are quite a few that sum two values.

Why is  _adapter_opt_f2i commented out?

a2l and a2i are strangely named and freaked me out at first but then I  
realized they are actually unbox operations.  maybe unboxi and unboxl?

tom

On Mar 10, 2009, at 3:48 PM, John Rose wrote:

> Thanks for the reviews so far.  I've posted the remainder of  
> meth.patch here:
>
> 6655638: dynamic languages need method handles
>  http://cr.openjdk.java.net/~jrose/6655638/webrev.00/  (not yet  
> integrated)
>
> (Long live cr.OJN!  Goodbye webrev.invokedynamic.info.)
>
> Later today (probably) I will update the index page to have useful  
> comments about how each file is changed.
>
> After responding to any further reviewer comments, I'll push the  
> results.
>
> Because it's a large body of code, and it will help to have a record  
> of what we do, let's keep review comments public.
>
> If the comments lead to large changes, I may have to ask for a re- 
> review.  If that happens, I'll publish at least a diff of webrev.N  
> to webrev.N+1, maybe even a 2nd-order webrev, if I can figure out how.
>
> About self-review:  I will be reviewing my own code, too, and if the  
> review period is long enough, may wish to make amendments.  I'll  
> email a public comment if this seems necessary.  (I've "discovered"  
> that people don't like it when they encounter unexplained changes  
> mid-review.)
>
> Meanwhile, I'm working on the same code that's being reviewed.  For  
> big reviews like this one, I'll try to keep separate workspaces for  
> review and further engineering.  (I wouldn't attempt this without hg/ 
> MQ and Emacs diff-mode.)  Therefore, if a review takes a long time  
> (which this one has) it may be followed immediately by a second  
> review cycle with a new bug ID, to push the adjustments separately.   
> In fact, I'll allocate the bug ID now:
>
> 6815692: method handle code needs some cleanup (post-6655638)
>  http://cr.openjdk.java.net/~jrose/6815692/webrev.00/  (not yet  
> posted)
>
> As that settles down I will be asking for reviews on this next step:
>
> 6655646: dynamic languages need dynamically linked call sites
>  http://cr.openjdk.java.net/~jrose/6655646/webrev.00/  (not yet  
> posted)
>
> After that, there will be individual pushes for additional assembly  
> code (x64 and/or sparc), compiler optimizations (2-3 steps there),  
> and various smaller things like compressed oops and cppInterpreter  
> support.  And of course bug fixes and cleanups as needed.
>
> Onward!
>
> -- John
>
> On Jan 20, 2009, at 2:53 AM, 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/
>