RFR JDK-8207194: [lworld] Update InnerClassLambdaMetafactory to add ValueTypes attribute in generated class

[mandy chung]

Hi Remi,

Thanks for the suggestion.  Here is the updated webrev:
   
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/8207194/webrev.01/index.html

On 7/16/18 6:51 AM, Remi Forax wrote:
> Hi Mandy,
> i think JavaLangAccess should export the array of the declared value types (the method in java.lang.Class should only do the null masking) and this array should be loaded by the constructor the builder and transformed to a set in the constructor too, because currently you are creating the set of declared value types at each call. Or even better, get the array of declared value types before creating the builder, because if the is not declared value type in the targetClass, there is no point to create the builder.
Good point.

I keep Class::getDeclaredValueTypeNames (renamed method to make it 
clear) to return an immutable set of Java class name rather than a 
mutable array with internal names so that the name conversion is done at 
most once if this method is called multiple times.
> The code
> while (c.isArray()) c = c.getComponentType();
>    if (c.isValue() && JLA.isDeclaredValueType(targetClass, c))
>      valueTypes.add(c.getName());
>
> can be extracted in it's own method add that takes a Class avoiding code duplication.

Sure and I planned to clean that up too.
> Instead of exporting the set of value types, i think it's better to use a method isEmpty because this is what you want.
>
> And the field JLA should be declared final because currently.
>
> Know correct me if i'm wrong, checking that the class is a value type at runtime is not necessary. Conceptually, you just want to propagate the fact that the targetClass believe that this is a value type to the code of the lambda proxy because the proxy class is generated at runtime and not at compile time. So this has nothing to do with isValue() which is a runtime check, so this is a kind of optimization.
> My fear here is that because of this optimization, the VM may not throw an exception where it should, i.e. if the class is declared as a value type but is not a value type at runtime, so if think the test to isValue() should be removed.

I think you are right.   The lambda proxy class should declare T in the 
ValueTypes attribute consistent with the target class regardless if T is 
a value type or not at runtime.  VM will do the value type consistency 
check.   I take out the isValue check.

Mandy
> regards,
> Rémi
>
> ----- Mail original -----
>> De: "mandy chung" <mandy.chung at oracle.com>
>> À: "valhalla-dev" <valhalla-dev at openjdk.java.net>
>> Envoyé: Samedi 14 Juillet 2018 03:14:08
>> Objet: RFR JDK-8207194: [lworld] Update InnerClassLambdaMetafactory to add ValueTypes attribute in generated class
>> VarHandleTest* and ValueConstructorRef tests fail with ICCE with
>> ValueTypes consistency checking because lambda generated class
>> is missing ValueTypes attribute whereas the target class has
>> the value types locally declared in the attribute.
>>
>> This patch updates InnerClassLambdaMetafactory to generate lambda
>> classes with ValueTypes attribute.  We will examine other class
>> file generators in JDK separately (JDK-8207315).  I also changed
>> ICCE to include the relevant info which is helpful for troubleshooting.
>> It may be useful to add a new method to return the list of declared
>> value types but leave it for another day.
>>
>> Webrev:
>>    http://cr.openjdk.java.net/~mchung/valhalla/webrevs/8207194/webrev.00
>>
>> I ran jdk_core, jdk_valhalla, and hotspot_valhalla test groups.
>>
>> Mandy