RFR: 8300926: Several startup regressions ~6-70% in 21-b6 all platforms [v4]

[daniel.daugherty at oracle.com]

On 2/23/23 9:54 PM, David Holmes wrote:
> On Thu, 23 Feb 2023 12:46:47 GMT, Robbin Ehn<rehn at openjdk.org>  wrote:
>
>>> I found one call to `add_to_hierarchy()` where we previously did not
>>> grab in `Compile_lock` in `vmClasses::resolve_shared_class()` and
>>> that call is made with this assert in place:
>>> ```assert(!Universe::is_fully_initialized(), "We can make short cuts only during VM initialization");```
>>>
>>> so it looks to me like we have a locking issue there (as in too early for locks).
>> I think you are taking about CDS dump process. It works fine taking the lock.
>> I think this was just some micro-optimization. But we should not mark and do deopt fixed that.
> To restate: the assert_locked_or_safepoint is pointless because you just took out the lock here.
>
> The code Dan points to does not seem to be related to CDS dump:
>
> bool vmClasses::resolve(vmClassID id, TRAPS) {
>    InstanceKlass** klassp = &_klasses[as_int(id)];
>
> #if INCLUDE_CDS
>    if (UseSharedSpaces && !JvmtiExport::should_post_class_prepare()) {
>      InstanceKlass* k = *klassp;
>      assert(k->is_shared_boot_class(), "must be");
>
>      ClassLoaderData* loader_data = ClassLoaderData::the_null_class_loader_data();
>      resolve_shared_class(k, loader_data, Handle(), CHECK_false);
>      return true;
>    }
> #endif // INCLUDE_CDS
>
> I remain concerned this code may be executed at a safepoint and we will now try to take the Compile_lock.
>
> -------------
>
> PR:https://git.openjdk.org/jdk/pull/12585

For some reason these comments no longer appear in the current PR.
That's probably because the code has changed relative to the original
anchors for the comments so the bots don't know where in the code to
attach the comments.

In any case, here's the current code that we were talking about (from
the v05 full webrev):


-void SystemDictionary::add_to_hierarchy(InstanceKlass* k) { +void 
SystemDictionary::add_to_hierarchy(JavaThread* current, InstanceKlass* 
k) { assert(k != nullptr, "just checking"); - if 
(Universe::is_fully_initialized()) { - 
assert_locked_or_safepoint(Compile_lock); - } + 
assert(!SafepointSynchronize::is_at_safepoint(), "must NOT be at 
safepoint"); + DeoptimizationScope deopt_scope; + { + MutexLocker 
ml(current, Compile_lock); So the 
"assert_locked_or_safepoint(Compile_lock)" call has been replaced by an 
unconditional: assert(!SafepointSynchronize::is_at_safepoint(), "must 
NOT be at safepoint"); which should trip if 
SystemDictionary::add_to_hierarchy() is ever called from a safepoint 
(with non-release bits). As far as I know, this assert has not fired in 
Robbin's testing. Also, the code that I was talking about was not "bool 
vmClasses::resolve(vmClassID id, TRAPS)". I was talking about 
"vmClasses::resolve_shared_class()" which is the code that calls this 
assert: assert(!Universe::is_fully_initialized(), "We can make short 
cuts only during VM initialization"); I was worried about this code path 
because its the only place that calls add_to_hierarchy() in the original 
code that _did not_ grab Compile_lock. With Robbin's latest code 
add_to_hierarchy() does grab Compile_lock, but if that was going to be a 
problem, it should have shown up in testing. Dan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-dev/attachments/20230224/419ed31b/attachment.htm>