Hi Daniil,
That seems fine to me.
Thanks,
David
On 16/05/2019 5:15 am, Daniil Titov wrote:
Hi David, Chris, and JC,
Please review a new version of the change that also fixes the similar problems in
ClassTypeImpl.subclasses(), InterfaceTypeImpl.subinterfaces(), and
InterfaceTypeImpl.implementors() methods. The fix moves the common code that iterates
over all loaded types while ignoring ObjectCollectedException in a new method
VirtualMachineImpl.forEachClass(Consumer<ReferenceType>).
Method ReferenceTypeImpl.nestedTypes() doesn't have this problem (since the
only method it calls on a reference type is name() that cannot throw
ObjectCollectedException()). However, to avoid potential pitfalls in the future
if someone decides to change this code, I modified this method to use the same
pattern as in the cases listed above.
All vmTestbase/nsk/jdi tests as well as tier1, tier2, and tier3 tests succeeded.
Webrev: http://cr.openjdk.java.net/~dtitov/8222422/webrev.02
Bug: https://bugs.openjdk.java.net/browse/JDK-8222422
Thanks,
Daniil
On 5/13/19, 2:59 PM, "David Holmes" <[email protected]> wrote:
Hi Daniil,
On 14/05/2019 5:56 am, Daniil Titov wrote:
> Hi David,
>
> It seems as the chances that class unloading happens during the
life-time of these tests are extremely low and switching Graal on increases these
chances. At least without Graal I could not locate any test result that showed
that ClassUnload event was posted. With Graal on 1 of 500 tests fails (at least
on Windows platform, on other platforms it must be more rare if not zero) and I
could not find any successful test showing that ClassUnload event was ever posted
for any class.
Interesting that only Graal exposes this ... though it may be that the
reflection accessor actually relates to Graal code, hence the reason the
unloading only shows up with Graal. It may mean there is a hole in our
test coverage with JDI - may need some tests that involve executing
reflection code with accessors eagerly generated so that we do see
class-unload events. Though timing would be problematic.
> You are right, the similar problem exists for ClassTypeImpl. subclasses() method and there is a separate issue for that: JDK-8223492. And while I was not able to reproduce it so far the logs provided in JDK-8223492 show that the problem here is the same. Initially I planned to keep these issues separated and proceed with JDK-8223492 after the current review is completed. But if you think it makes sense I could update the current webrev to include changes for ClassTypeImpl. subclasses() and then close JDK-8223492 as a duplicate.
Entirely up to you. But I would be suspicious of all of the code that
iterates vm.allClasses().
> I am also curious whether lambda forms, e.g. org/graalvm/compiler/lir/alloc/lsra/LinearScanLifetimeAnalysisPhase$$Lambda$93/1045689388, are supposed to be visible in JDI but I could not locate any discussion about this.
I'm trying to find that out too - but we can deal with that as a
separate issue.
Thanks,
David
> Thanks!
> --Daniil
>
>
> Per tests data and additional tracing it seems as with Graal on the
chances that class unloading happens during the test run are about 1/200 and only
on Windows platform. Without Graal there were no single occurrence of the test
when any ClassUnload event was posted.
>
> On 5/12/19, 3:19 PM, "David Holmes" <[email protected]> wrote:
>
> Hi Daniil,
>
> On 12/05/2019 3:14 am, Daniil Titov wrote:
> > Hi David,
> >
> > There are two ways how these reference types (for the classes
that become unloaded later) could appear in the collection
com.sun.tools.jdi.VirtualMachineImpl maintains and stores in
VirtualMachineImpl.typesBySignature instance variable:
> > 1) Initial load when all classes are requested from the debuggee
> > 2) Or added later when ClassPrepare event for a specific class is
posted and handled
> >
> > The reference types are removed from
VirtualMachineImpl.typesBySignature when ClassUnload event is processed. However,
additional tracing showed ( pleases see below the sample output) that in some cases
these ClassUnload events are received after we entered definedClasses() method and
started iterating over the copy of the collection of the classes returned by
vm.allClasses() method.
>
> Thanks for clarifying that for me. I was thinking in this case that
> definedClasses() was directly querying the target VM, but it isn't
it's
> iterating the existing set of known classes cached in the client
> (vm.allClasses()).
>
> Though it seems that whether or not we will hit this
> ObjectCollectedException depends on what we have already done with a
> particular ReferenceType. In this case we hit the exception when
> invoking classLoader() but that will only throw the exception if we
do
> not already have the classLoader cached and have to go and seek it
from
> the target VM.
>
> I do wonder why this issue should suddenly appear now? Encountering
an
> unloaded class, like a generated reflection accessor, should always
have
> been possible and so we should have seen this before. Something must
> have changed recently. ??
>
> I'm also concerned about other code that iterates through
> vm.allClasses() and which does not seem to be aware of the
possibility
> of CollectedObjectException. For example in ClassTypeImpl.java we
have:
>
> public List<ClassType> subclasses() {
> List<ClassType> subs = new ArrayList<>();
> for (ReferenceType refType : vm.allClasses()) {
> if (refType instanceof ClassType) {
> ClassType clazz = (ClassType)refType;
> ClassType superclass = clazz.superclass();
> if ((superclass != null) &&
superclass.equals(this)) {
> subs.add((ClassType)refType);
> }
> }
> }
>
> return subs;
> }
>
> If the superclass is already cached then this will work, but if it
has
> to call to the target VM over JDWP then we will have the same bug I
> think. Which again raises the question for me as to why we are not
> seeing tests fail here?
>
> > Based on the output below it seems as all unloaded classes are
the generated ones or lambda forms.
> >
> > --> debugger: ......getting: List definedClasses =
clRef.definedClasses();
> >
> > Received Unload Event for
Ljdk/internal/reflect/GeneratedConstructorAccessor1;
> > Handled Unload Event for
Ljdk/internal/reflect/GeneratedConstructorAccessor1;
>
> This is fine - generated reflection accessor are loaded in a custom
> classloader specifically so they can be unloaded promptly. But ...
>
> > Received Unload Event for
Lorg/graalvm/compiler/lir/alloc/lsra/LinearScanLifetimeAnalysisPhase$$Lambda$93/1045689388;
> > Handled Unload Event for
Lorg/graalvm/compiler/lir/alloc/lsra/LinearScanLifetimeAnalysisPhase$$Lambda$93/1045689388;
>
> ... these are I believe definitions of VM anonymous classes (they
have
> names of the form foo/<number> which are not legal class names).
Should
> these even be visible to JDI?
>
> Thanks,
> David
> -----
>
> > Received Unload Event for
Ljava/lang/invoke/LambdaForm$DMH/733189374;
> > Handled Unload Event for
Ljava/lang/invoke/LambdaForm$DMH/733189374;
> > Received Unload Event for
Lorg/graalvm/compiler/lir/alloc/lsra/LinearScanLifetimeAnalysisPhase$$Lambda$94/454340234;
> > Handled Unload Event for
Lorg/graalvm/compiler/lir/alloc/lsra/LinearScanLifetimeAnalysisPhase$$Lambda$94/454340234;
> > Received Unload Event for
Ljava/lang/invoke/LambdaForm$DMH/1780167778;
> > Handled Unload Event for
Ljava/lang/invoke/LambdaForm$DMH/1780167778;
> > Received Unload Event for
Lorg/graalvm/compiler/lir/alloc/lsra/LinearScanLifetimeAnalysisPhase$$Lambda$86/323001064;
> > Handled Unload Event for
Lorg/graalvm/compiler/lir/alloc/lsra/LinearScanLifetimeAnalysisPhase$$Lambda$86/323001064;
> >
> > Exception while getting classloader for type
jdk.internal.reflect.GeneratedConstructorAccessor1
> > # ERROR: ##> debugger: ERROR: Exception :
com.sun.jdi.ObjectCollectedException
> >
> >
> > Thanks!
> > --Daniil
> >
> >
> >
> > On 5/11/19, 3:39 AM, "David Holmes" <[email protected]>
wrote:
> >
> > On 11/05/2019 2:14 pm, Jean Christophe Beyler wrote:
> > > Isn't that the point? The list returned could have
unloaded classes and
> > > we can catch it via this exception (from the comment above
> > > the ReferenceType interface):
> > >
> > > * Any method on <code>ReferenceType</code> or which
directly or
> > > indirectly takes
> > > * <code>ReferenceType</code> as parameter may throw
> > > * {@link ObjectCollectedException} if the mirrored type
has been unloaded.
> > >
> > > Turns out that even in the definedClasses, we can get that
exception so
> > > we should check for it while walking the reference types
as well?
> >
> > I understand that the list returned to the "debugger"
process may
> > contain ReferenceTypes for types that have actually been
unloaded by the
> > time it queries them (unless the debuggee is suspended of
course). But I
> > don't see how we can encounter those types while compiling
the list in
> > the debuggee in the first place.
> >
> > Something seems amiss here ... possibly just my
understanding ...
> >
> > David
> >
> > > Jc
> > >
> > > *From: *Chris Plummer <[email protected]
> > > <mailto:[email protected]>>
> > > *Date: *Fri, May 10, 2019 at 9:09 PM
> > > *To: *David Holmes, Daniil Titov, OpenJDK Serviceability
> > >
> > > On 5/10/19 9:03 PM, Chris Plummer wrote:
> > > > On 5/10/19 8:59 PM, David Holmes wrote:
> > > >> Hi Daniil,
> > > >>
> > > >> On 11/05/2019 12:10 pm, Daniil Titov wrote:
> > > >>> Please review the change that fixes an
intermittent failure of the
> > > >>> test.
> > > >>>
> > > >>> The tests checks the implementation of the
> > > >>> com.sun.tools.jdi.ClassLoaderReference class. The
problem here is
> > > >>> that while
> > > >>>
com.sun.tools.jdi.ClassLoaderReferenceImpl.definedClasses()
> > > iterates
> > > >>> over all loaded classes to retrieve a classloader
and compares
> > > it to
> > > >>> the current one, some of the classes might become
unloaded and
> > > >>> garbage collected (e.g.
> > > >>>
org.graalvm.compiler.nodes.InliningLog$$Lambda$41.899832640 or
> > > >>>
jdk.internal.reflect.GeneratedConstructorAccessor1, etc.). If this
> > > >>> happens then the attempt to retrieve a
classloader for the
> > > collected
> > > >>> class results in
com.sun.jdi.ObjectCollectedException being thrown.
> > > >>
> > > >> That seems odd to me. If you have a reference to
the Class then it
> > > >> can't be unloaded. I would not expect allClasses()
to have
> > > >> weak-references, so a class should not be
unloadable while you are
> > > >> examining it. Unless it is finding VM anonymous
classes (which it
> > > >> should not!).
> > > >>
> > > > I was just typing up something similar. Shouldn't
the test do a
> > > > vm.suspend() and then call disableCollection() on
each class
> > > returned
> > > > by vm.allClasses()?
> > > Oh wait, this isn't a test. It's part of JDI. I guess
it would be up to
> > > the user of ClassLoaderReference.definedClasses() to
do the suspend. In
> > > fact I'm not sure there's much purpose in calling
> > > ClassLoaderReference.definedClasses() without
suspending first. Even
> > > with your changes, the list returned can end up with
references to
> > > unloaded classes.
> > >
> > > Chris
> > > >
> > > > Chris
> > > >> David
> > > >> -----
> > > >>
> > > >>> The fix catches this
com.sun.jdi.ObjectCollectedException and
> > > >>> continues iterating over the rest of the classes.
> > > >>>
> > > >>> Webrev:
http://cr.openjdk.java.net/~dtitov/8222422/webrev.01
> > > >>> Bug:
https://bugs.openjdk.java.net/browse/JDK-8222422
> > > >>>
> > > >>> Thanks!
> > > >>> --Daniil
> > > >>>
> > > >>>
> > > >
> > > >
> > >
> > >
> > >
> > >
> > > --
> > >
> > > Thanks,
> > > Jc
> >
> >
> >
>
>
>
