Hi David On Mon, May 28, 2018 at 8:42 AM, David Holmes <[email protected]> wrote: > Hi Thomas, > > On 28/05/2018 3:59 PM, Thomas Stüfe wrote: >> >> Hi David, >> >> On Mon, May 28, 2018 at 7:23 AM, David Holmes <[email protected]> >> wrote: >>> >>> Hi Thomas, >>> >>> I had a look at this and overall seems okay - the output looks good >> >> >> thanks! >>
<snip> >>> >>> Two queries: >>> >>> 1. Have we previously established whether a CSR request is needed for a >>> new >>> Dcmd? (My initial feeling is that it is.) >> >> >> My feeling is no, since this adds a new command, so there can be no >> backward compat issues. What is the general policy for new jcmd >> commands, or for that matter anything new added to the outside facing >> interface (new options, new Xlog tracing flags, changed output for >> existing options)? Do these things require CSR? > > > Generally yes. > > https://wiki.openjdk.java.net/display/csr/CSR+FAQs > > Q: What sort of changes require CSR review? > A: Any change to a JDK interface meant to be used outside of the JDK itself > requires CSR review. In this context "interface" isn't limited to the Java > programing language definition of an interface, but encompasses the broader > concept of a protocol between the JDK and users of the JDK. Examples of > interfaces by this definition include: > > Changes to public exported APIs in java.* and javax.* packages. > Changes to public and exported APIs in jdk.*packages. > New language updates to the Java Programming Language > New structures in the Java Virtual Machine Specification > Adding or removing a command in $JDK/bin > Adding, removing, or changing a command line option > Using or defining an environment variable > Using or defining a new file format or wire format > Changing or defining a new system or security property > > Interfaces that are experimental or for diagnostic purposes do not need to > go through CSR process, but the CSR process may be employed if feedback from > the CSR reviewers is desired. > --- > > IIRC (and I was hoping you may have recalled this :) ) last time this was > raised it was stated that as jcmd provides diagnostic commands that adding > or changing them doesn't need to go through the CSR process. > > Unfortunately I also found that such commands are documented: > > https://docs.oracle.com/javase/10/tools/jcmd.htm#GUID-59153599-875E-447D-8D98-0078A5778F05__DIAGNOSTICCOMMANDSFORJCMD-043BDB32 > > which may have an impact as it may mean we have to do something special to > get the documentation updated. :( Not sure. I would expect this to be a task for someone inside Oracle, since these documentations - to my understanding - refer to the Oracle JDK, not OpenJDK. Similar to how Redhat would have to polish up RHEL-relevant documentation once they take over new Fedora features. > > Anyway I think it is okay to proceed without a CSR request at this time, and > I/we can check on the documentation issue. > >> My problem with CSR is that it introduces a bottleneck, since it can >> only be approved by three very busy people at Oracle - if I understand >> the process right. Yes, we need a process to agree e.g. on syntax - >> desperately so, since e.g. sub option syntax in jcmd is a mess - but >> we seem to be strapped for reviewers even for normal code reviews, so >> the effect of creating a CSR in my experience is just a stop-of-work. > > > First note that a CSR request reviewer does not need to an OpenJDK Reviewer > - they simply need to be a competent engineer with an OpenJDK username who > basically sanity checks the CSR request to make sure it meets the expected > requirements as per the CSR documentation. > > Second, yes Joe tends to be the final approver, unless he delegates to > someone else when he is away. > > The expectation is that the need for a CSR request is established as one of > the first tasks when working on something, so that the request can be put in > and processed well ahead of the time you're ready to push. Again as per CSR > docs. In this case this is a hen-and-egg problem. If I assume I do not need a CSR I will only notice that I do once Reviewers tell me in the code review. But yes, there may be cases where I know beforehand that a CSR is required, I'll try to schedule CSR time for that in the future. > > But please raise any issues you have with the process with the CSR group - > as that is why it was created. mailto:[email protected] > Sure. >>> >>> 2. Is ClassLoaderHierarchyVMOperation a safepoint VM-op? I would expect >>> it >>> needs to be to be able to walk the CLD hierarchy, unless that is already >>> guaranteed to be safely walkable. Either way a comment clearly stating >>> that >>> would be useful I think. >> >> >> According to Coleen, CLDG can be walked outside a safepoint, but I did >> not want to risk it so I made it a safepoint operation (like other >> commands walking the CLDG, e.g. VM.metaspace). > > > Okay. Can you document that please. Will do. >>> >>> >>> Related to #2, is it really possible to encounter a CLD in the process of >>> being unloaded? Wouldn't that happen at a safepoint? >> >> >> Not sure, I am not a GC expert. I see places where this may be called >> concurrently, e.g. in the process of >> -XX:+ClassUnloadingWithConcurrentMark? >> >> Since a diagnostic command should never endanger the VM it monitors, I >> coded defensively. > > > Okay. Someone else may be able to clarify whether it is indeed possible or > not, but defensive is fine by me. > > Thanks, > David > > Thanks, Thomas >>> >>> Thanks, >>> David >>> >> >> Thank you, >> >> Thomas >> >>> >>> On 28/05/2018 2:50 PM, Thomas Stüfe wrote: >>>> >>>> >>>> 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 <[email protected]> >>>> 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
