On Thu, Aug 20, 2020 at 10:06 PM [email protected] <[email protected]> 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 <[email protected]> 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 > >
