[9] RFR (S): 8153540: C2 intrinsic for Unsafe.allocateInstance doesn't properly filter out array classes

[Vladimir Ivanov]

Thanks for the feedback, Vladimir.

>  > Unsafe.allocateInstance intrinsic can instantiate arrays, but the
> allocation logic is broken.
>
> But it should not allocate arrays. Right? This is what your java changes
> do now.
Yes, instance allocation logic doesn't work for arrays. It allocates 
broken array instances. That's why I decided to filter out arrays.

>
> Should it be allocateInstance0 ?:
>
> + // public native Object Unsafe.allocateInstance(Class<?> cls);

It is:
      @HotSpotIntrinsicCandidate
-    public native Object allocateInstance(Class<?> cls)
-        throws InstantiationException;
+    private native Object allocateInstance0(Class<?> cls) throws 
InstantiationException;

>
> You removed next stop check. Is it because java code will cat the NULL?:
>      Node* cls = null_check(argument(1));
>      if (stopped())  return true;
Yes, cls.isPrimitive() does null checks on both cls and klass.

>
> The test misses bug number @bug
Ok, will add.

Best regards,
Vladimir Ivanov

>
> Thanks,
> Vladimir K
>
> On 4/8/16 9:47 AM, Vladimir Ivanov wrote:
>> http://cr.openjdk.java.net/~vlivanov/8153540/webrev.00/hotspot/
>> http://cr.openjdk.java.net/~vlivanov/8153540/webrev.00/jdk/
>>
>> https://bugs.openjdk.java.net/browse/JDK-8153540
>>
>> Unsafe.allocateInstance intrinsic can instantiate arrays, but the
>> allocation logic is broken.
>>
>> The proposed fix is to perform necessary checks in Java code before
>> calling the intrinsic.
>>
>> I did some performance measurements [1] and reflection (non-constant
>> class) case (non-constant class) regressed ~5-10%
>> due to new guards added.
>>
>> I also experimented with a hotspot-only fix [2], but it looks uglier.
>> So, if you consider the regression in reflective
>> case non-critical, I'd prefer to go with JDK checks.
>>
>> Testing: regression test, JPRT, RBT (pit-hs-comp; in progress),
>> microbenchmarks.
>>
>> Thanks!
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> [1] http://cr.openjdk.java.net/~vlivanov/8153540/AllocInstance.java
>>
>> Baseline:
>>    AllocInstance.testConstant    avgt   25  3.736 ± 0.054  ns/op
>>    AllocInstance.testReflective  avgt   25  5.880 ± 0.080  ns/op
>>
>> JDK fix:
>>    AllocInstance.testConstant    avgt   25  3.959 ± 0.205  ns/op
>>    AllocInstance.testReflective  avgt   25  6.274 ± 0.180  ns/op
>>
>> [2] http://cr.openjdk.java.net/~vlivanov/8153540/webrev.slow_path
>>
>>    AllocInstance.testConstant avgt 25 3.957 ± 0.159 ns/op
>>    AllocInstance.testReflective avgt 25 5.901 ± 0.057 ns/op
>>