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

[Volker Simonis]

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://stackoverflow.com/questions/23632653/generate-java-heap-dump-on-uncaught-exception
[2] https://stackoverflow.com/questions/60887816/jvm-options-printnmtstatistics-save-info-to-file
[3] https://sudonull.com/post/25841-JVM-TI-how-to-make-a-plugin-for-a-virtual-machine-Odnoklassniki-company-blog
[4] https://2019.jpoint.ru/talks/2o8scc5jbaurnqqlsydzxv/

>
> 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