:) 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, "[email protected]" <[email protected]> wrote: On 8/21/20 11:07, [email protected] 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 [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://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 >>> >>> >
