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

[Attila Szegedi]

On Jan 10, 2018, at 2:32 PM, Hannes Wallnöfer <hannes.wallnoefer at oracle.com> wrote:
> 
> 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. 

Yeah, my solution wasn’t complete by any means, it just wanted to be a pointer in the right direction. Priya’s change to use IS_ARRAY *almost* worked, except for, as you noticed, primitive arrays.

FWIW, since IS_ARRAY isn’t actually used for anything else (the JIRA ticket allures to it getting retired), we could replace it with a new IS_REFERENCE_ARRAY ValidationType (that basically describes an instanceof Object[] check) and then Priya and mine combined solution below would work with it. The only other place where a new ValidationType needs to be handled is in AbstractJavaLinker.getGuard and GuardedInvocationComponent.Validator.compose. Those would need some more adjustments (basically, replacing every "validatorClass.isArray()” expression with “Object[].class.isAssignableFrom(validatorClass)”.)

Of course, then on another look it seems to me that length properties for collections and maps will *also* have stricter linkage than necessary, since INSTANCE_OF validation type will always link an instanceof check for the linked class, and these properties could also have more lenient linkage, like how element getters and setters are stably linked for lists and maps in general. So I guess we might need to add an IS_COLLECTION and IS_MAP ValidationType too to use for these length getters :-)

It’s not a big deal adding validation types, they’re just encapsulations of guard logic that also know how to compose (they need to compose in unnamed getter linking logic… welcome to the nontrivial world of bean linking :-) ). It might be tempting to just override getPropertyGetter() instead and add special code for linking “length”, and that’d _mostly_ work; one advantage of using setPropertyGetter() is that those properties defined with setPropertyGetter() will be automatically found by call sites with unnamed access,  e.g. Nashorn pseudocode:

var propName = (function() { return “length” })()
arr[propName]

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

Yeah, I’m also fine with whichever solution. Committing the reversal to Array.getLength is fine too with me.

It might be a nice exercise though to add IS_REFERENCE_ARRAY, IS_COLLECTION, and IS_MAP ValidationTypes, and define the length getters for arrays, collections, and maps using them, with requisite adjustments to both AbstractJavaLinker.getGuard and GuardedInvocationComponent.Validator.compose.

Attila.

> 
> 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
>> 
>