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

[forax at univ-mlv.fr]

> De: "mandy chung" <mandy.chung at oracle.com>
> À: "Remi Forax" <forax at univ-mlv.fr>
> Cc: "valhalla-dev" <valhalla-dev at openjdk.java.net>
> Envoyé: Lundi 16 Juillet 2018 21:18:29
> Objet: Re: RFR JDK-8207194: [lworld] Update InnerClassLambdaMetafactory to add
> ValueTypes attribute in generated class

> Hi Remi,

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

In Class. getDeclaredValueTypeNames(), at the end, you can returns 'names' instead of reloading the field declaredValueTypeNames from the memory (which is a volatile read). 
Given that you publish an immutable Set, i believe there is no need to declare declaredValueTypeNames as volatile here, if you remove it, at worst, each thread will need to recompute it once, in practice at some point the other threads will see the value. 

In the InnerClassLambdaFactory, isDeclaredValueType can be private. 

Otherwise, thumb up. 

Rémi 

> 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" [ mailto:mandy.chung at oracle.com | <mandy.chung at oracle.com> ]
>>> À: "valhalla-dev" [ mailto:valhalla-dev at openjdk.java.net |
>>> <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
>>> | http://cr.openjdk.java.net/~mchung/valhalla/webrevs/8207194/webrev.00 ] I ran
>>> jdk_core, jdk_valhalla, and hotspot_valhalla test groups.

>>> Mandy