for review (XXL): 6655638 method handles for invokedynamic

[John Rose]

On Mar 11, 2009, at 10:46 AM, Christian Thalinger wrote:
> As always, I try to do a review.

Thanks; more eyeballs on the code means better code.

> +  const int method_stack = (method->max_locals() + method- 
> >max_stack() + extra_stack) *
>                            Interpreter::stackElementWords();
> -  return overhead_size + method_stack + stub_code;
> +  return overhead_size + method_stack + extra_stack + stub_code;
>
> Adding extra_stack a second time on return seems wrong.

Good catch.  Would have broken the tagged-stack interpreter.

> +  product(bool, MethodHandleSupport,  
> false,                                 \
> +          "support method handles (true by default under JSR  
> 292")          \
>
> Closing ) is missing in the description.

Fixed.)

> 391   assert(methodOopDesc::invalid_vtable_index - 10 >  
> VM_INDEX_UNINITIALIZED, "Java sentinel value");
>
> At least to me it's not obvious why -10.

I added a comment.  The idea is to not even come close to overlapping  
with methodOopDesc::VtableIndexFlag, so that the Java and JVM code  
bases can move slightly without running into each others sentinel  
values.  I could have added another sentinel value to that enum, but  
then I'd have to couple it tightly with the method handle stuff, which  
means pushing the JVM's exact choice of value out to Java code.  The  
present setup is kludgy, but highly unlikely to break, especially with  
that aggressive assert in place.

> 539 void MethodHandles::expand_MemberName(Handle mname, int  
> suppress, TRAPS) {
>
> It seems suppress uses some hardcoded values.  It would be better to  
> use
> some defines or enum values.

Good idea.

> Seems the comments do not match the code, because rsi is not used in  
> the
> code.

rsi is used in the interpreter calling sequence (r13 for x64).
I put a comment referring to the usage elsewhere, and mentioned r13.

> One minor thing is that the copyright year is not updated in some (or
> all?) files.

Good point.  That is often corrected as part of our release  
engineering.  But I guess it's better to catch it early.

Thanks.  After I work over Tom's comments I'll repost the webrev.

-- John