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

[Vladimir Ivanov]

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/x86/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
>>>
>