On Jan 10, 2018, at 2:32 PM, Hannes Wallnöfer <[email protected]>
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
>> <[email protected]>:
>>
>> 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
>>>> <[email protected]> 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
>>
>
