RFR/RFA (M): 8185003: JMX: Add a version of ThreadMXBean.dumpAllThreads with a maxDepth argument
Hohensee, Paul
hohensee at amazon.com
Thu Sep 3 18:20:24 UTC 2020
Thanks for the re-review and approval, Volker. Now we just have to wait for the CSR to be approved.
I'm going to leave INT_MAX because that's the argument the original patch passes to jmm_DumpThreads via the dumpThreads0 native method in ThreadImpl.java.
Serguei, do you want to re-review?
On 9/3/20, 9:58 AM, "Volker Simonis" <volker.simonis at gmail.com> wrote:
On Tue, Sep 1, 2020 at 9:44 PM Hohensee, Paul <hohensee at amazon.com> wrote:
>
> 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/
>
Hi Paul,
this looks all good now (thanks for going the extra mile :)
I have just one small suggestion: when calling
"jmm_DumpThreadsMaxDepth(...)" from "jmm_DumpThreads(...)" in
"management.cpp" you can use "-1" instead of "INT_MAX" to get the
exact behaviour as before without having to rely on INT_MAX being
defined correctly. But I leave the decision up to you and even if you
update it, there's no need for a new webrev from my side.
Thumbs up and best regards,
Volker
> Thanks,
> Paul
>
> On 8/26/20, 12:22 PM, "jdk8u-dev on behalf of Hohensee, Paul" <jdk8u-dev-retn at openjdk.java.net on behalf of hohensee at amazon.com> 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" <volker.simonis at gmail.com> 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 <hohensee at amazon.com> 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, "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com> wrote:
> >
> > On 8/21/20 11:07, serguei.spitsyn at oracle.com 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 serguei.spitsyn at oracle.com
> > >> <serguei.spitsyn at oracle.com> 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 <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://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
> > >>>
> > >>>
> > >
> >
> >
>
>
More information about the jdk8u-dev
mailing list