for review (XXL): 6655638 method handles for invokedynamic

[Tom Rodriguez]

Looks good to me.

tom

On Apr 7, 2009, at 12:37 AM, John Rose wrote:

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