LGTM too :) *From: *Chris Plummer <[email protected]> *Date: *Wed, May 15, 2019 at 6:55 PM *To: *Daniil Titov, David Holmes, Jean Christophe Beyler *Cc: *OpenJDK Serviceability
LGTM > > Chris > > On 5/15/19 12:15 PM, 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- Thanks, Jc
