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