RFR JDK-8232222: Set state to 'linked' when an archived class is restored at runtime

[Ioi Lam]

Hi Jiangli,

I've asked this before. Do you have any performance data showing the 
benefit of the JVMTI part of the optimization, when JVMTI is used?

I think ultimately the JVMTI team should evaluate the patch. I CC'ed 
Serguei. I think it will be helpful if there's data that shows how JVMTI 
can benefit from this patch.

BTW, there's actually a race condition with the latest patch. I think 
this shows just how difficult it's to get things right in this very 
complicated part of the JVM.

1706     update_dictionary(d_hash, p_index, p_hash,
1707                       k, class_loader_h, THREAD);
1708   }
1709   k->eager_initialize(THREAD);
1710


 >>>> HERE

1711   // notify jvmti
1712   if (JvmtiExport::should_post_class_load()) {
1713       assert(THREAD->is_Java_thread(), "thread->is_Java_thread()");
1714       JvmtiExport::post_class_load((JavaThread *) THREAD, k);
1715
1716   }
1717   post_class_define_event(k, loader_data);
1718   if (k->is_shared() && k->is_linked()) {
1719     if (JvmtiExport::should_post_class_prepare()) {
1720       // To keep the same behavior as for dynamically loaded classes,
1721       // lock the init_lock before posting the ClassPrepare event.
1722       Handle h_init_lock(THREAD, k->init_lock());
1723       ObjectLocker ol(h_init_lock, THREAD, h_init_lock() != NULL);
1724       JvmtiExport::post_class_prepare((JavaThread *)THREAD, k);
1725     }
1726   }
1727 }

For a non-boot class, between the time where klass is added to the 
dictionary, to where you checked for ik->is_linked(), another thread 
could have called into klass->link_class_impl() and hence invoked the 
ClassPrepare callback. So your code will invoke the callback again.

Thanks
- Ioi


On 6/3/20 12:06 PM, Jiangli Zhou wrote:
> Hi Ioi,
>
> Really appreciate that you think through the details! I agree with
> your analysis about the serializing effect of '_init_lock' for posting
> class_prepare events. However I don't agree that an agent can rely on
> that for class hierarchy analysis as that's VM implementation specific
> behavior, but that is a different topic and does not belong to this
> thread.
>
> Let's analyze the runtime archived boot class loading behavior as
> well. When loading a shared class, the VM first loads all it's super
> types. There are multiple locks involved during loading a boot class.
> Those include the _system_loader_lock_obj (which is used for the boot
> loader as well) and the SystemDictionary_lock. These locks are held
> when loading a boot class by the NULL loader. That ensures the same
> serializing effect for posting class_prepare events after runtime
> restoration. Please let me know if you see any hole here.
>
> Thanks!
> Jiangli
>
> On Tue, Jun 2, 2020 at 10:46 PM Ioi Lam <ioi.lam at oracle.com> wrote:
>>
>>
>> On 6/2/20 10:16 PM, David Holmes wrote:
>>> Hi Ioi,
>>>
>>> On 3/06/2020 2:55 pm, Ioi Lam wrote:
>>>>
>>>> On 5/27/20 11:13 PM, David Holmes wrote:
>>>>> Hi Jiangli,
>>>>>
>>>>> On 28/05/2020 11:35 am, Ioi Lam wrote:
>>>>>>
>>>>>>> I was going to take the suggestion, but realized that it would add
>>>>>>> unnecessary complications for archived boot classes with class
>>>>>>> pre-initialization support. Some agents may set
>>>>>>> JvmtiExport::should_post_class_prepare(). It's worthwhile to support
>>>>>>> class pre-init uniformly for archived boot classes with
>>>>>>> JvmtiExport::should_post_class_prepare() enabled or disabled.
>>>>>> This would introduce behavioral changes when JVMTI is enabled:
>>>>>>
>>>>>> + The order of JvmtiExport::post_class_prepare is different than
>>>>>> before
>>>>>> + JvmtiExport::post_class_prepare may be called for a class that
>>>>>> was not called before (if the class is never linked during run time)
>>>>>> + JvmtiExport::post_class_prepare was called inside the init_lock,
>>>>>> now it's called outside of the init_lock
>>>>> I have to say I share Ioi's concerns here. This change will impact
>>>>> JVM TI agents in a way we can't be sure of. From a specification
>>>>> perspective I think we are fine as linking can be lazy or eager, so
>>>>> there's no implied order either. But this would be a behavioural
>>>>> change that will be observable by agents. (I'm less concerned about
>>>>> the init_lock situation as it seems potentially buggy to me to call
>>>>> out to an agent with the init_lock held in the first place! I find
>>>>> it hard to imagine an agent only working correctly if the init_lock
>>>>> is held.)
>>>> David,
>>>>
>>>> The init_lock has a serializing effect. The callback for a subclass
>>>> will not be executed until the callback for its super class has been
>>>> finished.
>>> Sorry I don't see that is the case. The init_lock for the subclass is
>>> distinct from the init_lock of the superclass, and linking of
>>> subclasses and superclasses is independent.
>>
>> In InstanceKlass::link_class_impl, you first link all of your super classes.
>>
>> If another thread is already linking your super class, you will block on
>> that superclass's init_lock.
>>
>> Of course, I may be wrong and my analysis may be bogus. But I hope you
>> can appreciate that this is not going to be a trivial change to analyze.
>>
>> Thanks
>> - Ioi
>>> David
>>> -----
>>>
>>>> With the proposed patch, the callback for both the super class and
>>>> subclass can proceed in parallel. So if an agent performs class
>>>> hierarchy analysis, for example, it may need to perform extra
>>>> synchronization.
>>>>
>>>> This is just one example that I can think of. I am sure there are
>>>> other issues that we have not thought about.
>>>>
>>>> The fact is we are dealing with arbitrary code in the callbacks, and
>>>> we are changing the conditions of how they are called. The calls
>>>> happen inside very delicate code (class loading, system dictionary).
>>>> I am reluctant to do the due diligence, which is substantial, of
>>>> verifying that this is a safe change, unless we have a really
>>>> compelling reason to do so.
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>>