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

[David Holmes]

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;
}

?

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