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.muthusw...@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.muthusw...@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
>
