[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR(s): 8203682: Add jcmd "VM.classloaders" command to print out class loader hierarchy, details
From:       coleen.phillimore () oracle ! com
Date:       2018-05-31 22:07:19
Message-ID: 334dd99b-dd25-169a-1ce8-7662109dadce () oracle ! com
[Download RAW message or body]


http://cr.openjdk.java.net/~stuefe/webrevs/8203682-jcmd-print-classloader-hierarchy/webrev.00/webrev/src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp.html


Can you make BranchTracker : public StackObj {} ? Are you going to move 
BranchTracker to utilities if you find other uses for it?

I don't like the use of mtInternal, but there doesn't seem to be a 
better category (or one to add).

  152     if (_loader_oop != NULL) {
  153       loader_klass = _loader_oop->klass();
  154       if (loader_klass != NULL) {
  155         loader_class_name = loader_klass->external_name();
  156       }
  157       oop nameOop = java_lang_ClassLoader::name(_loader_oop);
  158       if (nameOop != NULL) {
  159         const char* s = java_lang_String::as_utf8_string(nameOop);
  160         if (s != NULL) {
  161           loader_name = s;
  162         }
  163       }

These are available to the ClassLoaderData (_cld) as class_loader_name() 
and class_loader_klass().   I would rather see 
ClassLoaderData::loader_name() used - once fixed. Also, I don't like 
<unnamed> in the output.   It seems that most class loaders will be 
unnamed, so the output will be too noisy with that.   Also, there's an 
unnamed module so that makes it very confusing.

Since this is in a safepoint, maybe you can add a ResourceMark at 472, 
remove it from 362 and make all classes ResourceObj allocated. Then you 
can remove the code to delete the types.

Can you add assert that you are at a safepoint at the beginning of this, 
so you don't have to worry about LoaderTreeNode::_loader_oop getting 
collected. It seems that by saving _cld in LoaderTreeNode, that you 
don't need _loader_oop.

This code looks like it's walking the CLDG in order to make a different 
Class loader graph for printing.   Is it necessary to show the hierarchy 
information?

It seems like printing the address is better than <unnamed>, which is 
what I've looked at with the system dictionary logging.

http://cr.openjdk.java.net/~stuefe/webrevs/8203682-jcmd-print-classloader-hierarchy/we \
brev.00/webrev/test/hotspot/jtreg/serviceability/dcmd/vm/ClassLoaderHierarchyTest.java.html


What is JMXExecutor? Is this something to get in the way of debugging 
this test if it fails?

Sorry for the delay reviewing.
thanks,
Coleen


On 5/28/18 5:33 AM, Thomas Stüfe wrote:
> Hi Bernd,
> 
> definitely. I have two local patches here and corresponding RFEs open:
> https://bugs.openjdk.java.net/browse/JDK-8203343
> "VM.{metaspace|classloaders} jcmd should show invocation targets for
> Generated{Method|Constructor}AccessorImpl classes"
> https://bugs.openjdk.java.net/browse/JDK-8203849
> "DelegatingClassLoader name could be used to indicate reflected class"
> 
> Both complement each other and should make it possible to understand
> core reflection class loader issues better.
> 
> Nice idea about that folding, I can add this.
> 
> I plan to get them upstream for 11, but that may be illusory seeing
> how slow reviews are currently going. But certainly 12.
> 
> Gruss, Thomas
> 
> 
> On Mon, May 28, 2018 at 9:21 AM, Bernd Eckenfels <ecki@zusammenkunft.net> wrote:
> > Anything which can be done with the DelegatingClassLoaders (either add Info on \
> > their target or collabs them to a „30 x unnamed DelegatingClassloader". Even in \
> > Verbose mode they only reveal their type (constructor,  method) but not much \
> > more. 
> > Gruss
> > Bernd
> > --
> > http://bernd.eckenfels.net
> > ________________________________
> > From: serviceability-dev <serviceability-dev-bounces@openjdk.java.net> on behalf \
> >                 of Thomas Stüfe <thomas.stuefe@gmail.com>
> > Sent: Monday, May 28, 2018 6:50:34 AM
> > To: serviceability-dev@openjdk.java.net serviceability-dev@openjdk.java.net; \
> >                 Hotspot dev runtime
> > Subject: Re: RFR(s): 8203682: Add jcmd "VM.classloaders" command to print out \
> > class loader hierarchy, details 
> > All tests passed on jdk-submit.
> > 
> > Anyone interested in a review?
> > 
> > More output examples for jcmd VM.classloaders :
> > 
> > Spring framework, basic tree:
> > http://cr.openjdk.java.net/~stuefe/webrevs/8203682-jcmd-print-classloader-hierarchy/example_spring_short.txt
> >  
> > Spring framework, including all classes:
> > http://cr.openjdk.java.net/~stuefe/webrevs/8203682-jcmd-print-classloader-hierarchy/example_spring_long.txt
> >  
> > ... Thomas
> > 
> > On Wed, May 23, 2018 at 2:46 PM, Thomas Stüfe <thomas.stuefe@gmail.com> wrote:
> > > Dear all,
> > > 
> > > (not sure if this would be a serviceability or runtime rfe, so sorry
> > > for crossposting)
> > > 
> > > may I please have feedback/reviews for this small enhancement.
> > > 
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8203682
> > > Webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8203682-jcmd-print-classloader-hierarchy/webrev.00/webrev/
> > >  
> > > This adds a new command to jcmd, "VM.classloaders". It complements the
> > > existing command "VM.classloader_stats".
> > > 
> > > This command, in its simplest form, prints the class loader tree. In
> > > addition to that, it optionally prints out loaded classes (both
> > > non-anonymous and anonymous) and various classloader specific
> > > information.
> > > 
> > > Examples:
> > > 
> > > http://cr.openjdk.java.net/~stuefe/webrevs/8203682-jcmd-print-classloader-hierarchy/example.txt
> > >  http://cr.openjdk.java.net/~stuefe/webrevs/8203682-jcmd-print-classloader-hierarchy/example-with-classes.txt
> > >  http://cr.openjdk.java.net/~stuefe/webrevs/8203682-jcmd-print-classloader-hierarchy/example-with-reflection-and-noinflation.txt
> > >  
> > > 
> > > Thanks and Best Regards,
> > > 
> > > Thomas


[prev in list] [next in list] [prev in thread] [next in thread]