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

[serguei.spit...@oracle.com]

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?

Thanks,
Serguei


On 8/20/20 11:30, Volker Simonis wrote:

On Wed, Aug 19, 2020 at 6:17 PM Hohensee, Paul <hohen...@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