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

[Christian Thalinger]

> On Apr 11, 2016, at 1:50 AM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> wrote:
> 
> Thanks for the feedback, Aleksey.
> 
>>> I did some performance measurements [1] and reflection (non-constant
>>> class) case (non-constant class) regressed ~5-10% due to new guards added.
>> 
>> My quick perfasm runs seems to show this is because a subtle difference:
>>   http://cr.openjdk.java.net/~shade/8153540/baseline.perfasm
>>   http://cr.openjdk.java.net/~shade/8153540/patched.perfasm
>> 
>> If you compare these, then the difference seems to be the instruction
>> scheduling and a branch in the guards code.
>> 
>> Baseline:
> ...
>> Patched:
> ...
>> 
>> Unfortunately, a simple fix of replacing "||" with "|" explodes the
>> generated code. Maybe something else is doable there.
> 
> Yes, C2 can't fuse Class.isArray with slow bit check from Klass::layout_helper. (Partly, because they dispatch to different places: !Class.isArray() case dispatches to explicit exception instantiation and slow path calls into runtime).
> 
> Additional flag in a mirror (j.l.Class) which marks instance klasses could help here, but I'm still not sure it's worth the effort.
> 
> Ideally, something like [1] (which requires 2 new intrinsics):

I would advise against that.  We are fixing a long-standing bug here and although we see a regression the code we produced before was just wrong.  Comparing against something that was wrong in the first place is moot.

Take the hit; I doubt it will show up at customer applications.

> 
>  * isFastAllocatable() performs all necessary checks: null checks on cls, not primitive, not array, not interface, not abstract, fully initialized, no finalizers;
> 
>  * allocateInstanceSlow() handles all cases the intrisic doesn't handle: either throws IE or does necessary operations (e.g., initialize the class or register a finalizer) when instantiating an object.
> 
> Best regards,
> Vladimir Ivanov
> 
> [1]
>    @ForceInline
>    public Object allocateInstance(Class<?> cls) throws InstantiationException {
>        // Interfaces and abstract classes are handled by the intrinsic.
>        if (isFastAllocatable(cls)) {
>            return allocateInstance0(cls);
>        } else {
>            return allocateInstanceSlow(cls);
>        }
>    }
> 
>    @HotSpotIntrinsicCandidate
>    private native boolean isFastAllocatable(Class<?> cls);
> 
>    @HotSpotIntrinsicCandidate
>    private native Object allocateInstance0(Class<?> cls) throws InstantiationException;
> 
>    // Calls into modified OptoRuntime::new_instance_C
>    @HotSpotIntrinsicCandidate
>    private native Object allocateInstanceSlow(Class<?> cls) throws InstantiationException;
> 
> 
>> 
>>> [1] http://cr.openjdk.java.net/~vlivanov/8153540/AllocInstance.java
>> 
>> Suggestions to improve fidelity:
>>   * Run allocation benchmarks with -Xmx1g -Xms1g; this improves variance
>>   * Add @CompilerControl(CompilerControl.Mode.DONT_INLINE) on
>> @Benchmarks if you want to use -prof perfasm
>> 
>> Thanks,
>> -Aleksey
>> 
>>