[13] RFR (M): 8223216: C2: Unify class initialization checks between new, getstatic, and putstatic

[dean.long at oracle.com]

OK, thanks for the explanation.  Looks good.

dl


On 5/3/19 3:49 PM, Vladimir Ivanov wrote:
> Thanks for the feedback, Dean.
>
>> Do you want to have a Runtime reviewer take a look at the new logic?
>
> I'm definitely looking for feedback on 8223213 from Runtime team. But 
> 8223216 is C2-specific and incrementally builds on top of it, so I 
> don't think there's anything new for Runtime team to look at.
>
>> Can you explain why Parse::clinit_deopt() changed from testing for
>>
>> InstanceKlass::fully_initialized
>>
>> to testing for
>>
>> InstanceKlass::being_initialized
>>
>> instead?  How do we know we it is the initializing thread?
>
> Initializing thread is irrelevant here. The check is solely about the 
> current state of the holder class.
>
> Parse::clinit_deopt() is not mandatory (nmethod clinit barrier on 
> entry cover all important cases), but an optimization. It is added by 
> 8223213 specifically for C2 to trigger recompilation once the holder 
> class is fully initialized. The motivation is to get better code when 
> a class is fully initialized.
>
> The change in 8223216 is intended as a refactoring: since there are 
> only 2 states allowed here (being_initialized and fully_initialized), 
> it doesn't matter what state is checked (== being initialized vs != 
> fully_initialized).
>
> Best regards,
> Vladimir Ivanov
>
>> On 5/1/19 4:37 PM, Vladimir Ivanov wrote:
>>> http://cr.openjdk.java.net/~vlivanov/8223216/webrev.00/
>>> https://bugs.openjdk.java.net/browse/JDK-8223216
>>>
>>> (The patch has minor dependencies on 8223213 [1] I sent out for 
>>> review earlier.)
>>>
>>> C2 implements class initialization checks for new and 
>>> getstatic/putstatic differently: while "new" supports fast class 
>>> initialization checks, static field accesses rely on uncommon traps 
>>> which may lead to deoptimization/recompilation storms during 
>>> long-running class initialisation.
>>>
>>> Proposed patch unifies implementation between them and uses the 
>>> following barrier:
>>>    if (holder->is_initialized()) {
>>>      uncommon_trap(initialized, reinterpret);
>>>    }
>>>    if (!holder->is_reentrant_initialization(current_thread)) {
>>>      uncommon_trap(uninitialized, none);
>>>    }
>>>
>>> It also enhances checks for not-yet-initialized classes 
>>> (Compile::needs_clinit_barrier) and unifies the implementation 
>>> between new, invokestatic, and getfield/putfield.
>>>
>>> Testing: tier1-5, targeted microbenchmarks, new test from 8223213
>>>
>>> Thanks!
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>> [1] http://cr.openjdk.java.net/~vlivanov/8223213/webrev.00/
>>> https://bugs.openjdk.java.net/browse/JDK-8223213
>>>
>>