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

Tue Jun 9 18:06:23 UTC 2020

On Mon, Jun 8, 2020 at 6:36 PM Ioi Lam <ioi.lam at oracle.com> 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?
>
Hi Ioi,

The performance gain for this change is a constant (for any fixed
number of loaded classes), with or without JVMTI agent, hope you are
able to understand it. The startup data that I provided was without a
JVMTI agent. However the same saving applies when an agent exists, as
I've pointed out earlier already. I'll run a comparison with agent
enabled and provide it to the community.

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

The serviceability-dev at openjdk.java.net mailing list has been CC'ed
and I've asked for Serguei or Chris to comment in one of the earlier
emails.

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

Please don't mix this change with non-boot class. This change only
handles the archived boot class.

Thanks,
Jiangli

> 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.lam at 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
> >>>>
> >>>>
>