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.phillim...@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#L880Also, 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, JiangliOn Thu, Jun 4, 2020 at 3:14 PM Jiangli Zhou <jiangliz...@google.com> wrote:Hi David,On Wed, Jun 3, 2020 at 9:59 PM David Holmes <david.hol...@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 tobe 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, JiangliDavid ----- 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:The point is that there is already a race in terms of the execution ofOn 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 have to say I share Ioi's concerns here. This change will impactI was going to take the suggestion, but realized that it would addunnecessary 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 thanbefore+ 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_lockJVM 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 potentiallybuggy to me to call out to an agent with the init_lock held in thefirst 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 subclasswill 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 blockon that superclass's init_lock.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.Yes I agree. While in general ordering of the class_prepare callbacksOf 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.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 evaluatea-priori. So as you say we should have a fairly good reason for doingthis. I can easily envisage that pre-linking when no callbacks are enabled would be a performance boost. But with callbacks enabled andconsuming CPU cycles any benefit from pre-linking could be lost in thenoise. What if we did as Ioi suggested and only set the class as linked inrestore_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, butalso shortens the link time path when the callbacks are enabled. Hope I got that right. :) David -----Thanks - IoiDavid -----With the proposed patch, the callback for both the super class andsubclass 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
