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

List:       openjdk-serviceability-dev
Subject:    Re: RFR(s): 8203682: Add jcmd "VM.classloaders" command to print out class loader hierarchy, details
From:       Thomas_Stüfe <thomas.stuefe () gmail ! com>
Date:       2018-05-28 9:50:52
Message-ID: CAA-vtUxW+tKrc6uq9XtB9=mA2zJrgS=MYi4RuqmX2kRyctQpPw () mail ! gmail ! com
[Download RAW message or body]

Hi David

On Mon, May 28, 2018 at 8:42 AM, David Holmes <david.holmes@oracle.com> wro=
te:
> Hi Thomas,
>
> On 28/05/2018 3:59 PM, Thomas St=C3=BCfe wrote:
>>
>> Hi David,
>>
>> On Mon, May 28, 2018 at 7:23 AM, David Holmes <david.holmes@oracle.com>
>> 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 itse=
lf
> requires CSR review. In this context "interface" isn't limited to the Jav=
a
> programing language definition of an interface, but encompasses the broad=
er
> 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 t=
o
> go through CSR process, but the CSR process may be employed if feedback f=
rom
> 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 addin=
g
> 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 t=
o
> 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 Review=
er
> - they simply need to be a competent engineer with an OpenJDK username wh=
o
> basically sanity checks the CSR request to make sure it meets the expecte=
d
> 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 C=
SR
> 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:csr-discuss@openjdk.java.net
>

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 alread=
y
>>> 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 o=
r
> 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=C3=BCfe 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-classloa=
der-hierarchy/example_spring_short.txt
>>>>
>>>> Spring framework, including all classes:
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8203682-jcmd-print-classloa=
der-hierarchy/example_spring_long.txt
>>>>
>>>> ... Thomas
>>>>
>>>> On Wed, May 23, 2018 at 2:46 PM, Thomas St=C3=BCfe <thomas.stuefe@gmai=
l.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-classlo=
ader-hierarchy/webrev.00/webrev/
>>>>>
>>>>> This adds a new command to jcmd, "VM.classloaders". It complements th=
e
>>>>> 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-classlo=
ader-hierarchy/example.txt
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8203682-jcmd-print-classlo=
ader-hierarchy/example-with-classes.txt
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8203682-jcmd-print-classlo=
ader-hierarchy/example-with-reflection-and-noinflation.txt
>>>>>
>>>>>
>>>>> Thanks and Best Regards,
>>>>>
>>>>> Thomas
[prev in list] [next in list] [prev in thread] [next in thread]