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

Tue Jun 9 01:47:35 UTC 2020

There's also another race condition that the callback for a subclass 
will be called *before* the callback of its superclass.

On 6/8/20 6:36 PM, Ioi Lam wrote:
> 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
>>>>>
>>>>>
>