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

[Jiangli Zhou]

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