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

[Doerr, Martin]

Hi Vladimir,

thanks for doing this change.


Unfortunately, I had missed that the assertions don't work with -Xcomp:
assert(method->holder()->is_being_initialized() || method->holder()->is_initialized(), ...

LIR_Assembler::clinit_barrier (x86)
MachPrologNode::emit (x86)
Parse::clinit_deopt() (C2 code)

Assertion failures can be reproduced by running
jck tests vm/constantpool/Initialization/Initialization010/Initialization01001m051/Initialization/*
with -Xcomp.
They fail in C1 and if you use -XX:-TieredCompilation in C2.


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()?


Sorry that these issues didn't come into my mind earlier.

Best regards,
Martin



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