Hi, I've re-finalized the CSR after Volker's re-review. See https://bugs.openjdk.java.net/browse/JDK-8251498. It now says we won't update JMM_VERSION. I've also updated the webrevs to reflect the CSR changes and to target 8u282.
jdk: http://cr.openjdk.java.net/~phh/8185003/webrev.8u.jdk.07/ hotspot: http://cr.openjdk.java.net/~phh/8185003/webrev.8u.hotspot.07/ Thanks, Paul On 8/26/20, 12:22 PM, "jdk8u-dev on behalf of Hohensee, Paul" <[email protected] on behalf of [email protected]> wrote: +Joe for an opinion. I agree. I've added a comment to the CSR (https://bugs.openjdk.java.net/browse/JDK-8251498) and moved it back to Draft. "Volker Simonis has pointed out in https://mail.openjdk.java.net/pipermail/jdk8u-dev/2020-August/012557.html that when we backport a JMM feature, we're actually updating the existing JMM version specification rather than transitioning to a new one. There was a bit of recognition of this in https://bugs.openjdk.java.net/browse/JDK-8249101, where the @since javadoc tag was updated to 11.0.9 rather than 14. Imo, Volker makes a good argument for leaving the JMM version alone when doing JMM backports. If we adopt this approach, the JDK 11 JMM version should also be reverted." Thanks, Paul On 8/26/20, 10:18 AM, "Volker Simonis" <[email protected]> wrote: Hi Paul, thanks for adapting your change. Please find my comments in-line below: On Tue, Aug 25, 2020 at 10:28 PM Hohensee, Paul <[email protected]> wrote: > > :) > > New webrevs following Volker's suggestion. > > http://cr.openjdk.java.net/~phh/8185003/webrev.8u.jdk.06/ Looks good except JNI_OnLoad() in "management.c" where I'd change the call to "JVM_GetManagement(JMM_VERSION)" back to "JVM_GetManagement(JMM_VERSION_1_0)". See discussion below... > http://cr.openjdk.java.net/~phh/8185003/webrev.8u.hotspot.06/ > Looks good except Management::get_jmm_interface(): 2396 if (version == JMM_VERSION) { 2397 return (void*) &jmm_interface; 2398 } You still check for "JMM_VERSION" which is now "0x20020000" and thus incompatible with the old value of "JMM_VERSION_1 = 0x20010000". This will break compatibility with clients compiled against jmm.h before this change. It should therefore remain unchanged: if (version == JMM_VERSION_1_0) { return (void*) &jmm_interface; } I think the variant "if (version == JMM_VERSION_1_0 || version == JMM_VERSION)" which we've briefly discussed wouldn't work either because a binary "JMM_VERSION_2" client would expect that "DumpThreads" will have the additional "maxDepths" argument and crash. So we can't have binary compatibility with both, old jdk8 clients and new jdk11 clients. Therefore, contrary to my previous mail, I'd also change "jmm_GetVersion()" to return the "old" JMM VERSION (i.e "0x20010203") because that's really the only one we're compatible with. In fact, this makes the whole addition of "JMM_VERSION_2" questionable, because after the proposed changes it wouldn't be used anymore. And after reasoning about it a little more, I think that's correct because we really only have binary compatibility with previous jdk8 clients and that's how it should be. Thank you and best regards, Volker > 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 > >>> > >>> > > > >
