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

[David Holmes]

Correction ...

On 3/06/2020 5:19 pm, David Holmes wrote:
> On 3/06/2020 3:44 pm, Ioi Lam 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.
> 
> The point is that there is already a race in terms of the execution of 
> the two callbacks. So while this change can certainly produce a 
> different result to what would previously be seen, such a result is 
> already possible in the general case.
> 
>> 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.
> 
> Yes I agree. While in general ordering of the class_prepare callbacks is 
> not guaranteed for independent classes, if a given application 
> explicitly loads and links classes in a known order then it can 
> (reasonably) expect its callbacks to execute in that order. If this 
> change means classes will now be linked in an order independent of what 
> the normal runtime order would be then that could be a problem for 
> existing agents.
> 
> So where does this leave us? The change is within spec, but could 
> trigger changes in agent behaviour that we can't really evaluate 
> a-priori. So as you say we should have a fairly good reason for doing 
> this. I can easily envisage that pre-linking when no callbacks are 
> enabled would be a performance boost. But with callbacks enabled and 
> consuming CPU cycles any benefit from pre-linking could be lost in the 
> noise.
> 
> What if we did as Ioi suggested and only set the class as linked in 
> restore_unshareable_info if !JvmtiExport::should_post_class_prepare(); 
> and in addition in InstanceKlass::link_class_imp we added an additional 
> check at the start:
> 
> // Pre-linking at load time may have been disabled for shared classes,
> // but we may be able to do it now.
> if (JvmtiExport::should_post_class_prepare() &&
>      !BytecodeVerificationLocal &&
>      loader_data->is_the_null_class_loader_data()) {
>    _init_state = linked;
> }

There should obviously be a check for is_shared() in there as well.

David
-----

> ?
> 
> That avoids the problem of changing the JVM TI callback behaviour, but 
> also shortens the link time path when the callbacks are enabled.
> 
> Hope I got that right. :)
> 
> David
> -----
> 
>> 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
>>>>
>>>>
>>