for review (XXL): 6655638 method handles for invokedynamic

[John Rose]

Thanks for going over the method handle stuff, Tom.  It needed it.

The updated webrev, hopefully the final one, is here:
   http://cr.openjdk.java.net/~jrose/6655638/webrev.03/

The difference between webrev.02 and webrev.03 is here; it contains  
everything described below in this email:
   http://cr.openjdk.java.net/~jrose/6655638/02-to-03/

My responses are below.  Please me know if you have further comments,  
or if I missing the point of something you said.

If all is well enough, I hope to push soon and start a round on review  
on invokedynamic proper.

-- John

On Mar 31, 2009, at 1:08 PM, Tom Rodriguez wrote:

> 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.

Changed to MethodHandleEntry.  (That was already in webrev.02, did you  
see it?)

> CHECK_UNHANDLED_OOPS might be worth running at some point.

Good idea; will do.

> 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); }

I'll put the braces in the usual place on those guys.  (The CHECK and  
THROW macros are a pain sometimes.)

> 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; }

I'll expand the uses of the local throw functions and remove them.

I think it was mainly a convenience for prototyping; it was a pain  
matching the THROW_X macro to the enclosing return type.  You are  
right about the usefulness of the file//line info.  I'll add the  
missing THROW_NULL and THROW_MSG_NULL to exceptions.hpp; that was the  
main irritant.

Actually, after refactoring the adapter verification logic (see  
below), I kept one one for making InternalErrors with formatted  
message strings.

> 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?

There are lots of little functions that don't block and  do something  
small, usually parse a data structure.  The data structures have lots  
of indirections, so there are lots of temporary oops flying around.   
In this case, handles don't add value; they just slow things down.

I'll make a cleaner separation between trapping functions that take  
handles, and non-trapping ones that don't.  And I'll comment it.

Trapping functions that I'll change to take all handles:  
verify_method_type.
Non-trapping ones that I'll change to take all oops:  
check_method_receiver.
Trapping ones I changed to not trap:  all the verify_method_type_*  
functions.

I found a few places with oop-nulling and I made the code there more  
handle-oriented.

> adapter_conversion could use some asserts validating the encoding.

Yes, I'll DTRT there.

> 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?

Good idea.  No sense encouraging bit leakage.

> 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.

OK, I went ahead and factored all three init_XMH guys to call a  
verify_XMH with at least the main chunk of verification logic  
contained in it.

> I know in some cases that might be difficult but  
> init_AdapterMethodHandle in particular has a chunk that might be  
> movable.

That's very doable, and the code looks better that way.  Thanks.

> 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?

OK.

> 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.

Done.

> Don't do the (int32_t)NULL_WORD cast here:
>     __ cmpptr(arg_slots.as_register(), (int32_t) NULL_WORD);

Right.

> 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?

The relevant assert is against _INSERT_NO_MASK, which says that the  
caller has explicitly assumed responsibility for inserting the tag bits.

I made this clearer by moving the TSI-related code all into one block.

> 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.

Good idea; done.

> 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()));

Yes.  Actually, I'll change them to use load_klass; will make the LP64  
port a little easier.

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

Yes, they are gone.

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

Yeah, well.  Having three different places for the adapter sequence  
stinks, actually.  I changed it to use two enumerations, one for the  
JVM and one that bridges to the native Java API.  The EntryKind  
enumeration is now what all the switches key off of, without addition  
expressions in the cases.  I kept the bridge to Java in  
impl_java_dyn_AdapterMethodHandle (with some tweaks to make it look  
even more like the Java defs).  The AdapterKind guy, which was neither  
one nor the other, is gone.  Is that better?

> Why is  _adapter_opt_f2i commented out?

Because I haven't gotten around to implementing it.

> 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?

Good idea.  It was a weak parallel with l2i, etc., but it's better to  
emphasize what's going on.

Done.