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

Tue Jun 9 12:20:27 UTC 2020

Hi Jiangli,  I'm sorry I didn't see the whole thread because I filtered 
it to serviceability-dev.  I see you have answered some of these 
questions, there. Still reading.

Coleen

On 6/8/20 10:46 PM, coleen.phillimore at oracle.com wrote:
>
> Hi Jiangi,
>
> I apologize for jumping in at this late stage of this change. I've 
> seen the emails but there's been a lot of discussion which is hard to 
> follow.
>
> I have some concerns with setting the state to "linked" since the 
> changes that Erik Osterlund is working on would require reinitializing 
> the itable and vtables when you load the shared class.  See the JEP 
> for New Invoke Bindings 
> https://bugs.openjdk.java.net/browse/JDK-8221828. 
> <https://bugs.openjdk.java.net/browse/JDK-8221828>
> We would have to remove this optimization.  Erik is planning on 
> getting this work into JDK 16 since we have a functionally complete 
> version.  See:
> https://github.com/coleenp/jdk/blob/erik-calls/src/hotspot/share/oops/instanceKlass.cpp#L880 
>
>
> Also, I haven't figured out why you are enabling this optimization if 
> JVMTI is requested, since the optimization seems to have minor 
> benefits.  And I'm concerned with threads observing the class as 
> linked but I don't see any bugs there.  By setting the state to 
> "linked" we are skipping these steps:
>
> linking super classes and interfaces - can we assume that they are 
> already linked when ik->restore_unshareable_info is called ?
> check_verification_constraints - presumably OK for NULL CLD, this has 
> a quick exit
> link_methods - this is already called in restore_unshareable_info so 
> it has a quick exit
> check_linking_constraints - presumably OK for NULL CLD, this has a 
> quick exit
> initializing the vtable - will need to revert the change for new 
> invoke bindings and maybe valhalla.
>
> link_class_impl doesn't really do that much for boot class loader.
>
> I imagine that this change is so that potentially CDS classes can be 
> pre-initialized so that more in the mirror can be shared, which sounds 
> difficult to do except maybe for some classes.  Is this being 
> discussed on the Project Leyden thread?
>
> Thanks,
> Coleen
>
>
> On 6/8/20 9:02 PM, Jiangli Zhou wrote:
>> Hi Ioi,
>>
>> After incorporating David's suggestion of locking init_lock for
>> posting ClassPrepare events, do you have other concerns about the
>> change? I hope we are finally able to move on with an inclusive and
>> right solution that works for broad usages, particular on the cloud
>> spectrum.
>>
>> Best,
>> Jiangli
>>
>> On Thu, Jun 4, 2020 at 3:14 PM Jiangli Zhou <jianglizhou at google.com> 
>> wrote:
>>> Hi David,
>>>
>>> On Wed, Jun 3, 2020 at 9:59 PM David Holmes 
>>> <david.holmes at oracle.com> wrote:
>>>> Ioi pointed out that my proposal was incomplete and that it would need
>>>> to be more like:
>>>>
>>>> if (is_shared() &&
>>>>       JvmtiExport::should_post_class_prepare() &&
>>>>       !BytecodeVerificationLocal &&
>>>>       loader_data->is_the_null_class_loader_data()) {
>>>>       Handle h_init_lock(THREAD, init_lock());
>>>>       ObjectLocker ol(h_init_lock, THREAD, h_init_lock() != NULL);
>>>>       set_init_state(linked);
>>>>       >>> call JVMTI
>>>>       return true;
>>>>     }
>>>>
>>>> This alleviates any concerns about behavioural changes to JVM TI, and
>>>> also allows JVM TI enabled code to partially benefit from the
>>>> pre-linking optimisation.
>>>>
>>>> Otherwise I agree with Ioi that any behaviour change to JVM TI 
>>>> needs to
>>>> be justified by significant performance gains.
>>>>
>>> Thanks a lot for the input and suggestion! Locking the init_lock for
>>> the JVMTI ClassPrepre event here sounds ok to me. The ClassDefine is
>>> normally posted before the ClassPrepare. That's why the change was
>>> made in systemDictionary.cpp instead of within
>>> InstanceKlass::restore_unshareable_info() function, to keep the same
>>> events ordering for any given class. I added the 'init_lock' locking
>>> code for post_class_prepare(), and kept the code in
>>> systemDictionary.cpp in webreve.03 below.  Not changing the JVMTI
>>> events ordering feels safer to me. Would the following be ok to
>>> everyone?
>>>
>>>    http://cr.openjdk.java.net/~jiangli/8232222/webrev.03/
>>>
>>> I also changed the InstanceKlass::restore_unshareable_info() to set
>>> _init_state via set_init_state API as you suggested. We can get away
>>> without locking the init_lock for setting the flag itself.
>>>
>>> Best regards,
>>>
>>> Jiangli
>>>
>>>
>>>> David
>>>> -----
>>>>
>>>> On 4/06/2020 8:42 am, David Holmes wrote:
>>>>> 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
>>>>>>>>>
>>>>>>>>>
>