RFR/RFA (M): 8185003: JMX: Add a version of ThreadMXBean.dumpAllThreads with a maxDepth argument

Tue Aug 25 20:28:26 UTC 2020

:)

New webrevs following Volker's suggestion.

http://cr.openjdk.java.net/~phh/8185003/webrev.8u.jdk.06/
http://cr.openjdk.java.net/~phh/8185003/webrev.8u.hotspot.06/

Passes

jdk/test/com/sun/management
jdk/test/java/lang/management
jdk/test/sun/management
jdk/test/javax/management

Paul

On 8/21/20, 1:39 PM, "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com> wrote:

    On 8/21/20 11:07, serguei.spitsyn at oracle.com wrote:
    > Hi Paul,

    Sorry, Volker, for using this "indirection".
    I hope, Paul redirected my "Hi" to you. :)

    Thanks,
    Serguei

    >
    > Thank you for explanation.
    >
    > Thanks,
    > Serguei
    >
    >
    > On 8/21/20 10:54, Volker Simonis wrote:
    >> On Thu, Aug 20, 2020 at 10:06 PM serguei.spitsyn at oracle.com
    >> <serguei.spitsyn at oracle.com> wrote:
    >>> Hi Paul,
    >>>
    >>> I was also wondering if there is a compatibility risk involved with
    >>> the JMM_VERSION change.
    >>> So, thanks to Volker for asking these questions.
    >>>
    >>> One more question.
    >>> I do not see a backport of the
    >>> src/jdk.management/share/native/libmanagement_ext/management_ext.c
    >>> change.
    >>> Is it intentional, and if so, what is the reason to skip this file?
    >>>
    >> "libmanagement_ext/management_ext.c" doesn't exist in jdk8. It was
    >> introduced with "8042901: Allow com.sun.management to be in a
    >> different module to java.lang.management" in jdk9. In jdk8 all the
    >> functionality is in "management/management.h" so there's no need to
    >> backport the changes from "management_ext.c" .
    >>
    >> [1] https://bugs.openjdk.java.net/browse/JDK-8042901
    >>
    >>> Thanks,
    >>> Serguei
    >>>
    >>>
    >>> On 8/20/20 11:30, Volker Simonis wrote:
    >>>
    >>> On Wed, Aug 19, 2020 at 6:17 PM Hohensee, Paul <hohensee at amazon.com>
    >>> wrote:
    >>>
    >>> Please review this backport to jdk8u. I especially need a CSR
    >>> review, since the CSR approval process can be a bottleneck. The
    >>> patch significantly reduces fleet profiling overhead, and a version
    >>> of it has been in production at Amazon for over 3 years.
    >>>
    >>>
    >>>
    >>> Original JBS issue: https://bugs.openjdk.java.net/browse/JDK-8185003
    >>>
    >>> Original CSR: https://bugs.openjdk.java.net/browse/JDK-8185705
    >>>
    >>> Original patch:
    >>> http://hg.openjdk.java.net/jdk10/master/rev/68d46cb9be45
    >>>
    >>>
    >>>
    >>> Backport JBS issue: https://bugs.openjdk.java.net/browse/JDK-8251494
    >>>
    >>> Backport CSR: https://bugs.openjdk.java.net/browse/JDK-8251498
    >>>
    >>> Backport JDK webrev:
    >>> http://cr.openjdk.java.net/~phh/8185003/webrev.8u.jdk.05/
    >>>
    >>> JDK part looks good to me.
    >>>
    >>> Backport Hotspot webrev:
    >>> http://cr.openjdk.java.net/~phh/8185003/webrev.8u.hotspot.05/
    >>>
    >>> HotSpot part looks good to me but see discussion below.
    >>>
    >>>
    >>> Details of the interface changes needed for the backport are in the
    >>> Description of the Backport CSR 8251498. The actual functional
    >>> changes are minimal and low risk.
    >>>
    >>> I've also reviewed the CSR yesterday which I think is fine. But now,
    >>> when looking at the implementation, I'm a little concerned about
    >>> changing JMM_VERSION from " 0x20010203" to "0x20020000" in "jmm.h".
    >>>
    >>> This might be especially problematic in combination with the changes
    >>> in "Management::get_jmm_interface()" which is called by
    >>> JVM_GetManagement():
    >>>
    >>>   void* Management::get_jmm_interface(int version) {
    >>>   #if INCLUDE_MANAGEMENT
    >>> -  if (version == JMM_VERSION_1_0) {
    >>> +  if (version == JMM_VERSION) {
    >>>       return (void*) &jmm_interface;
    >>>     }
    >>>   #endif // INCLUDE_MANAGEMENT
    >>>     return NULL;
    >>>   }
    >>>
    >>> You've correctly fixed the single caller of "JVM_GetManagement()" in
    >>> the JDK (in "JNI_OnLoad()" in "management.c"):
    >>>
    >>> -    jmm_interface = (JmmInterface*)
    >>> JVM_GetManagement(JMM_VERSION_1_0);
    >>> +    jmm_interface = (JmmInterface*) JVM_GetManagement(JMM_VERSION);
    >>>
    >>> but I wonder if there are other monitoring/serviceability tools out
    >>> there which use this interface and which will break after this change.
    >>> A quick search revealed at least two StackOverflow entries which
    >>> recommend using "JVM_GetManagement(JMM_VERSION_1_0)" [1,2] and there's
    >>> a talk and a blog entry doing the same [3, 4].
    >>>
    >>> I'm not sure how relevant this is but I think a much safer and
    >>> backwards-compatible way of doing this downport would be the
    >>> following:
    >>>
    >>> - don't change "Management::get_jmm_interface()" (i.e. still check for
    >>> "JMM_VERSION_1_0") but return the "new" JMM_VERSION in
    >>> "jmm_GetVersion()". This won't break anything but will make it
    >>> possible for clients to detect the new version if they want.
    >>>
    >>> - don't change the signature of "DumpThreads()". Instead add a new
    >>> version (e.g. "DumpThreadsMaxDepth()/jmm_DumpThreadsMaxDepth()") to
    >>> the "JMMInterface" struct and to "jmm_interface" in "management.cpp".
    >>> You can do this in one of  the two first, reserved fields of
    >>> "JMMInterface" so you won't break binary compatibility.
    >>> "jmm_DumpThreads()" will then be a simple wrapper which calls
    >>> "jmm_DumpThreadsMaxDepth()" with Integer.MAX_VALUE as depth.
    >>>
    >>> - in the jdk you then simply call "DumpThreadsMaxDepth()" in
    >>> "Java_sun_management_ThreadImpl_dumpThreads0()"
    >>>
    >>> I think this way we can maintain full binary compatibility while still
    >>> using the new feature. What do you think?
    >>>
    >>> Best regards,
    >>> Volker
    >>>
    >>> [1]
    >>> https://urldefense.com/v3/__https://stackoverflow.com/questions/23632653/generate-java-heap-dump-on-uncaught-exception__;!!GqivPVa7Brio!LDD5rfKbGz6KCl0LqcAgrFq7kNLkkoDhhN0ZSgHMDvgGMY5bvKJdpoXIAK6N-KqVsyaF$
    >>> [2]
    >>> https://urldefense.com/v3/__https://stackoverflow.com/questions/60887816/jvm-options-printnmtstatistics-save-info-to-file__;!!GqivPVa7Brio!LDD5rfKbGz6KCl0LqcAgrFq7kNLkkoDhhN0ZSgHMDvgGMY5bvKJdpoXIAK6N-Ip7MAQ5$
    >>> [3]
    >>> https://urldefense.com/v3/__https://sudonull.com/post/25841-JVM-TI-how-to-make-a-plugin-for-a-virtual-machine-Odnoklassniki-company-blog__;!!GqivPVa7Brio!LDD5rfKbGz6KCl0LqcAgrFq7kNLkkoDhhN0ZSgHMDvgGMY5bvKJdpoXIAK6N-ErSjPdD$
    >>> [4]
    >>> https://urldefense.com/v3/__https://2019.jpoint.ru/talks/2o8scc5jbaurnqqlsydzxv/__;!!GqivPVa7Brio!LDD5rfKbGz6KCl0LqcAgrFq7kNLkkoDhhN0ZSgHMDvgGMY5bvKJdpoXIAK6N-Oxb5CQ-$
    >>>
    >>> Passes the included (suitably modified) test, as well as the tests in
    >>>
    >>>
    >>>
    >>> jdk/test/java/lang/management/ThreadMXBean
    >>>
    >>> jdk/test/com/sun/management/ThreadMXBean
    >>>
    >>>
    >>>
    >>> Thanks,
    >>>
    >>> Paul
    >>>
    >>>
    >