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

[coleen.phillimore at oracle.com]

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