RFR:8157251:BeanLinker relinks array length operations for array types

[Hannes Wallnöfer]

Priya, 

The problem with your solution that using an object array invocation with a primitive array will throw a ClassCastException as it passes the isArray guard.

On the other hand, the problem with instanceof validation in Attila’s snippet is that it uses the concrete callsite type (e.g. String[]) instead of Object[]. What we’d need is a guard with an instanceof Object[] check. 

However, I don’t think a callsite for array length will usually see so many different array classes to go megamorphic, so I think both your solutions (Priya’s webrev and Attila’s code snippet) are acceptable.

Hannes

> Am 10.01.2018 um 06:42 schrieb Priya Lakshmi Muthuswamy <priya.lakshmi.muthuswamy at oracle.com>:
> 
> Hi Attila,
> 
> Thanks for the review. I just felt in the case of primitive types there will be relinking.
> I tried with the below fix.
> In the case object types, I see relinking when we use Validation.INSTANCE_OF. But works fine without relinking when we use Validation.IS_ARRAY
> 
> if(clazz.isArray()) {
>    // Some languages won't have a notion of manipulating collections. Exposing "length" on arrays as an // explicit property is beneficial for them. if (clazz.getComponentType().isPrimitive()) {
>        setPropertyGetter("length", MethodHandles.arrayLength(clazz), ValidationType.EXACT_CLASS);
>    }else {
>        setPropertyGetter("length", MethodHandles.arrayLength(Object[].class), ValidationType.IS_ARRAY);
>    }
> 
> Thanks,
> Priya
> 
> 
> On 1/9/2018 7:51 PM, Attila Szegedi wrote:
>> This effectively reverts the combined <https://bugs.openjdk.java.net/browse/JDK-8157225> and <https://bugs.openjdk.java.net/browse/JDK-8157250>.
>> 
>> That was not the intent of 8157251, though.
>> 
>> The intent of 8157251 was to use MethodHandles.arrayLength(Object[].class) for all arrays with components of reference type (and use ValidationType.INSTANCE_OF for it, as all such arrays will be instance-of Object[] through instance-of semantics for array types) and leave the linking of arrays with primitive component types as they are today.
>> 
>> So, basically, something like:
>> 
>> if(clazz.isArray()) {
>>     // Some languages won't have a notion of manipulating collections. Exposing "length" on arrays as an
>>     // explicit property is beneficial for them.
>>     if (clazz.getComponentType().isPrimitive()) {
>>         setPropertyGetter("length", MethodHandles.arrayLength(clazz), ValidationType.EXACT_CLASS);
>>     } else {
>>         setPropertyGetter("length", MethodHandles.arrayLength(Object[].class), ValidationType.INSTANCE_OF);
>>     }
>> 
>> Obviously, this is again a tradeoff between linkage stability and performance, except that with this solution we don’t sacrifice any performance and we still increase linkage stability. Going back to Array.getLength does further increase linkage stability, but *speculatively* it doesn’t intrinsify to as lean code as MethodHandles.arrayLength does (it certainly erases the type information of the argument); someone with time on their hands should probably run some PrintAssembly tests with -XX:-TieredCompilation to see whether this is true (if not, then 8157225 and 8157250 were done for nothing and this patch undoes them anyway).
>> 
>> In any case, I don’t have a too strong opinion about this; I don’t mind even if this ends up being a reversal of 8157225; it’s just weird that we have arrayLength method handle intrinsics and not use them.
>> 
>> Attila.
>> 
>> 
>>> On Jan 9, 2018, at 8:00 AM, Priya Lakshmi Muthuswamy <priya.lakshmi.muthuswamy at oracle.com> wrote:
>>> 
>>> Hi,
>>> 
>>> Please review JDK-8157251 : BeanLinker relinks array length operations for array types
>>> 
>>> JBS : https://bugs.openjdk.java.net/browse/JDK-8157251
>>> webrev : http://cr.openjdk.java.net/~pmuthuswamy/8157251/webrev.00/
>>> 
>>> Thanks,
>>> Priya
>