[13] RFR (M): 8223213: Implement fast class initialization checks on x86-64

[Vladimir Ivanov]

Thanks, Martin.

I filed JDK-8225106 [1] and sent out the fix for review [2]

> And after thinking longer about it, I think that it's not ideal that we check is_being_initialized() several times in C2 (recheck in GraphKit::clinit_barrier for deoptimization).
> Should we better enforce consistency by only checking clinit_barrier_on_entry()?

I introduced ciInstanceKlass::is_not_initialized() (mirrors 
InstanceKlass::is_not_initialized()) to properly narrow cases when class 
initialization has been started. I'd prefer to keep the asserts in-place.

Best regards,
Vladimir Ivanov

[1] https://bugs.openjdk.java.net/browse/JDK-8225106[2] 
https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-May/034058.html

>> -----Original Message-----
>> From: hotspot-dev <hotspot-dev-bounces at openjdk.java.net> On Behalf Of
>> Vladimir Ivanov
>> Sent: Donnerstag, 30. Mai 2019 12:38
>> To: hotspot-dev developers <hotspot-dev at openjdk.java.net>
>> Subject: Re: [13] RFR (M): 8223213: Implement fast class initialization checks
>> on x86-64
>>
>> Thanks for reviews, Vladimir, Claes, David, Martin, and Coleen!
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> On 29/05/2019 22:06, coleen.phillimore at oracle.com wrote:
>>>
>>> Vladimir,
>>>
>>> This looks good to me.
>>>
>>> On 5/29/19 12:20 PM, Vladimir Ivanov wrote:
>>>> Thanks, Coleen.
>>>>
>>>> Updated webrev:
>>>>    http://cr.openjdk.java.net/~vlivanov/8223213/webrev.03
>>>>
>>>> Incremental webrev:
>>>>    http://cr.openjdk.java.net/~vlivanov/8223213/webrev.03_02/
>>>>
>>>>> I reviewed mostly the interpreter and shared change.  As someone else
>>>>> commented, I don't like the addition of a develop flag because some
>>>>> platforms don't support class initialization barriers.  Isn't it
>>>>> normal to do this with the misnamed VM_Version, like adding
>>>>> VM_Version::supports_class_initialization_barriers() with x86
>>>>> returning true until the other platforms false until they implement
>>>>> the feature. Then there isn't another flag configuration to test (or
>>>>> not test).
>>>>
>>>> I like your suggestion. Didn't know VM_Version is used in such a way.
>>>>
>>>> Replaced UseFastClassInitChecks with
>>>> VM_Version::supports_fast_class_init_checks().
>>>>
>>>>>
>> http://cr.openjdk.java.net/~vlivanov/8223213/webrev.02/src/hotspot/cpu/x
>> 86/interp_masm_x86.cpp.udiff.html
>>>>>
>>>>>
>>>>> I have to admit that the relationship between resolved bytecode in
>>>>> bytecode_1/bytecode_2 and which _f1/_f2 held the Method* was
>> actually
>>>>> a surprise to me.  There's nothing structurally in cpCache other than
>>>>> reading the code that enforces this and it wasn't always a Method* in
>>>>> f2 for invokeinterface, for example, so it's sort of an accident.
>>>>>
>>>>> But this code is correct and I think as a follow up we should make
>>>>> load_invoke_cp_cache_entry() call load_resolved_method_at_index()
>> too
>>>>> and have some assert in cpCache that this is true, or rewrite the
>>>>> cpCache completely.
>>>>
>>>> I went ahead and changed load_invoke_cp_cache_entry() to call
>>>> load_resolved_method_at_index() (along with some other minor
>>>> refactorings). If you have any ideas/suggestions about the assert, I
>>>> can add it as well.
>>>
>>> I think the change to use load_resolved_method_at_index() is good
>>> because if someone moves things around, it'll now fail very quickly.  I
>>> think this is the right amount of refactoring.
>>>
>>> Thanks,
>>> Coleen
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
>>>>
>>>>> The code to do the initialization barrier in the interpreter looks good.
>>>>>
>>>>> Thanks,
>>>>> Coleen
>>>>>
>>>>> On 5/28/19 7:40 AM, Vladimir Ivanov wrote:
>>>>>> Thanks, Martin.
>>>>>>
>>>>>> Updated webrev:
>>>>>>    http://cr.openjdk.java.net/~vlivanov/8223213/webrev.02/
>>>>>>
>>>>>>> Are these assertions safe?
>>>>>>> +   assert(method()->needs_clinit_barrier(), "barrier not needed");
>>>>>>> +   assert(method()->holder()->is_being_initialized(), "barrier not
>>>>>>> needed");
>>>>>>> Can it happen that initialization concurrently completes before
>>>>>>> they are evaluated?
>>>>>>
>>>>>> Good point. Even though ciInstanceKlass caches initialization state
>>>>>> of the corresponding InstanceKlass, it seems there's a possibility
>>>>>> that the state is updated during the compilation (see
>>>>>> ciInstanceKlass::update_if_shared). I enhanced the asserts to check
>>>>>> that initialization has been stated.
>>>>>
>>>>> Ok, this makes sense.
>>>>>>
>>>>>>> A small suggestion for x86 TemplateTable::invokeinterface:
>>>>>>> It'd be nice to replace load of interface klass by your new
>>>>>>> load_method_holder.
>>>>>>
>>>>>> Agree. Updated.
>>>>>
>>>>> This is nice.
>>>>>
>>>>>
>>>>>>
>>>>>> Best regards,
>>>>>> Vladimir Ivanov
>>>>>
>>>