for review (XXL): 6655638 method handles for invokedynamic

[John Rose]

On Mar 11, 2009, at 7:10 PM, Tom Rodriguez wrote:

> I've done all the easy stuff and I'm onto the methodHandles* files  
> but I thought I'd give you these first.

Thank you!

> MethodHandleSupport isn't a great flag name since it sounds like a  
> thing.  SupportMethodHandles, EnableMethodHandles or  
> AllowMethodHandles would read better I think.

OK:  EnableMethodHandles it is.


> method_entry(invoke_method) should be method_entry(method_handle) I  
> think.

True.  C++ interpreter support isn't in there yet.  I have to look  
again at platforms not yet supported and comment out some of those  
tentative changes.

> assembler_sparc.cpp:
>
> I'd be tempted to put the delay slot nop in  
> jump_to_method_handle_entry instead of expecting the caller to do  
> it.  Otherwise I think it needs to a comment mentioning the need to  
> fill the delay.  I know the other jump_to leave it up to the caller  
> but they are all pretty much single instruction.   
> jump_to_method_handle_entry does a bit of work.  Either way is fine.

You are right; it looks funny for a big macro to end with an unfilled  
delay slot.  The other guys look like pseudo-instructions.
I'll put the nop inside the macro, and later on if I ever get tempted  
to optimize it, I'll add an optional boolean:
   void jump_to_method_handle_entry(Register mh_reg, Register temp_reg,
                                    // pass true here if you don't  
want the final nop:
                                    bool caller_adds_delay_slot =  
false);

jump_indirect_to is just slightly more like a pseudo-instruction, so  
let's leave it like jmp, jmpl, etc.


> this is a pretty bizarre way to call throw_if_not_2
>
> __ throw_if_not_x(Assembler::never,
>
> I know it's used in other places, I think it would be cleaner to  
> just inline the needed throw_if_not_2 code.  It's just 2 assembly  
> instructions.  I find the juxtaposition of if and never a little  
> jarring when in fact it means always throw.

I like the way the "__ throw..." macros read.  But for the 'always'  
case a naked jump is just as informative, given that the stub name has  
"throw" in it.

> What's this for:
>
> +#undef  __
> +#define __ _masm->
> +

Leftovers from an earlier arrangement of the code.  I nuked it.

> SymbolPropertyEntry and SymbolPropertyTable could use at least a  
> small comment describing their intended purpose.

Good idea; done.

> all of the variants of this:
>
> +  assert(Klass::cast(mname->klass())- 
> >is_subclass_of(SystemDictionary::MemberName_klass()), "wrong type");
>
> could simply be:
>
> assert(is_instance(mname), "wrong type");

Thanks!  Done, for several variants.

> method_type_pointer_chase could use some comments.  maybe a  
> different name would be better since it's not actually a pointer  
> chase but the offsets to use in a pointer chase.   
> method_type_offsets_chain?

OK.

>
> What units is methodOopDesc::extra_stack() in?  It says slots but to  
> me that means slots in the vmreg/optoreg sense which is 32 bit words  
> but I guess it means interpreter slots.  It should have a more  
> precise comment and maybe the name could reflect it somehow.  I  
> guess it's in the same units as max_stack and max_locals.

Yes. "extra_stack" by analogy with "max_stack".

> Maybe extra_stack_entries() would get that across.

OK.  This will make it clear, and give me a chance to go back and look  
for bugs:
   static int extra_stack_entries() { return EnableMethodHandles ?  
(int)MethodHandlePushLimit : 0; }
   static int extra_stack_words()   { return extra_stack_entries() *  
Interpreter::stackElementSize(); }

>
> The methodHandles part is to come.

Good deal.

-- John