review request (L): 7001379: bootstrap method data needs to be moved from constant pool to a classfile attribute

[John Rose]

On Nov 25, 2010, at 2:27 AM, Christian Thalinger wrote:

> On Nov 25, 2010, at 8:42 AM, John Rose wrote:
>> On Nov 22, 2010, at 6:57 PM, John Rose wrote:
>> 
>>> Because of a strong and reasonable request from the IBM JVM team, the JSR 292 class file format for CONSTANT_InvokeDynamic is changing one more time.
>>> ...
>>> I will be putting out a review request soon for this.
>> 
>> 
>> Here is a JVM change which moves the variadic parts of CONSTANT_InvokeDynamic specifiers out of the constant pool and into a new attribute, BootstrapMethods.
>> http://cr.openjdk.java.net/~jrose/7001379/webrev.hs.00
> 
> src/share/vm/classfile/classFileParser.hpp:
> 
> +  //@@  static int start_operand_group(GrowableArray<int>* &operands, int op_count, TRAPS);
> +  //@@  static void store_operand_array(GrowableArray<int>* operands, constantPoolHandle cp, TRAPS);
> 
> Do we still need them?

Oops, nope.

> src/share/vm/oops/constantPoolOop.hpp:
> 
> -  void copy_entry_to(int from_i, constantPoolHandle to_cp, int to_i, TRAPS);
> +  static void copy_entry_to(constantPoolHandle from_cp, int from_i, constantPoolHandle to_cp, int to_i, TRAPS);
> 
> Maybe I'm missing the obvious here but why do need to pass a handle in?  There is nothing fancy going on in that method.

In principle, the call to 'klass_at' could trigger a GC.  We know it won't, because of the tag of the CP entry.  But that requires a private, invisible contract with the present implementation of 'klass_at'.  It is safer to use the handle.  Since I had to introduce a handle in copy_cp_to_impl, it was natural to move it down into the per-entry routine.

>> For review, here is the specification change, in the javadoc in the jdk repo:
>> http://cr.openjdk.java.net/~jrose/7001379/webrev.jdk.00
> 
> 301  * Each {@code CONSTANT_InvokeDynamic} entry refers has a reference to a
> 302  * bootstrap method specifier defined in a separate array.
> 
> I think there are some superfluous words.

Thanks.  Fixed and clarified:

 * Each {@code CONSTANT_InvokeDynamic} entry contains an index which references
 * a bootstrap method specifier; all such specifiers are contained in a separate array.
 * This array is defined by a class attribute named {@code BootstrapMethods}.

> 317  * {@code BootstrapMethods} attribute.  In particular, each cosntant
> 
> Typo.

Fixed.

>> The JDK webrev also includes an adjusted unit test.
>> 
>> The base revision was reviewed by Christian yesterday:
>> http://cr.openjdk.java.net/~jrose/7001423/webrev.00
>> 
>> The JVM code passes the unit test, of course.
> 
> 
> -- Christian

Thanks.  Can I cite you as a reviewer on both change sets?

-- John