for review (XXL): 6655638 method handles for invokedynamic

[John Rose]

On Feb 12, 2009, at 8:55 AM, Christian Thalinger wrote:

> On Tue, 2009-01-20 at 02:53 -0800, John Rose wrote:
>> Please give it a look.
>
> I have now reviewed all files.  At least I tried to understand what's
> happening but methodHandles.{cpp,hpp} is too much, sorry.
>
> src/share/vm/classfile/classFileParser.cpp:
>
> ClassFileParser::java_dyn_MethodHandle_fix_pre()
>
> (*fields_ptr) is used 7 times, readability would be much better if  
> that
> value would be assigned to a local variable.

Yes, I wouldn't have done it that way, except that I'm following the  
lead of existing code (java_lang_ref_Reference_fix_pre).  Shall we  
leave it as-is, or fix both?  I could bind a local variable like this:
	typeArrayHandle& fields = (*fields_ptr);

> src/share/vm/oops/instanceKlass.cpp:
>
> instanceKlass::is_same_package_member_impl
>
> +     klassOop next = outer1->compute_enclosing_class(ignore_name,  
> CHECK_(false));
> +     klassOop next = outer2->compute_enclosing_class(ignore_name,  
> CHECK_(false));
>
> There is a define CHECK_false for CHECK_(false).

Thanks.

> src/share/vm/prims/methodHandles.cpp:
>
> void MethodHandles::set_enabled(bool z) {
>
> This method can also disable the method handle, right?  Wouldn't it be
> clearer if there is a set_enabled() (or enable()) and a set_disabled()
> (disable()) method (without an argument)?

This is a recurrent issue with boolean accessors.  Hotspot internal  
APIs are strongly regular in their use of this pattern for property  
access:
   T foo();
   void set_foo(T x);

If T is bool, sometimes we have irregularity in the setter name:
   void set_foo(bool x);
   void set_is_foo(bool x);
   void set_has_foo(bool x);

It is less common to split a boolean setter in two as you suggest:
   T is_foo();
   void set_foo();
   void clear_foo();

There are cases where the setting is a state transition, as with  
CompileTask::mark_complete with CT::is_complete.  In such cases, there  
are two accessor functions, so the split seems natural.

Some classes have the full three-way pattern, as with  
JavaThread::is_suspend_equivalent.  That introduces three functions  
where two will do about as well.

I guess I prefer not to use the three-way scheme, since it requires a  
little too much inventiveness in picking positive and negative  
versions of a property name (e.g., enable vs. disable, set vs.  
clear).  It makes it harder for non-English-speakers to read the  
sources, and I'm thinking, of course, of /bin/grep.  :-)

I prefer the present formulation as simplest and most neutral in  
connotations.  But if there's a strong reason to change it in this  
case, I'll do it.

> Typo in line 538:
> // which refers dirctly to JVM internals.

Thanks.

> void MethodHandles::expand_MemberName(Handle mname, int suppress,  
> TRAPS) {
>
>        //Handle name = java_lang_String::create_from_symbol(m- 
> >name(), CHECK);
>        Handle name = StringTable::intern(m->name(), CHECK);
>
> Is that intentional?

Yes.  I'll comment it better.

Thanks!

-- John