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

[David Holmes]

Answering via email ...

On 25/02/2023 6:11 am, daniel.daugherty at oracle.com wrote:
> 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.

Yes it gets very hard to keep track of things.

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

Okay that partially relieves my concern. Obviously whomever put the 
original assert_or_locked did believe/expect it to be called from a 
safepoint. It would be good to understand if they were mistaken or 
whether things have changed, or whether testing simply isn't catching 
that case.

> 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

Yes and in response to your concern Robbin said that there was no issue 
at CDS dump time. I then showed that vmClasses::resolve called 
vmClasses::resolve_shared_class() not during CDS dump - hence it was 
potentially still a concern.

David
-----