Do we want to increment the JMM version by one whenever we change the spec regardless of JDK version? If so, we'd have
JMM_VERSION_2 = 0x20020000, // JDK 10 JMM_VERSION_3 = 0x20030000, // JDK 14 JMM_VERSION = JMM_VERSION_3 On 9/24/19, 9:54 AM, "Mandy Chung" <mandy.ch...@oracle.com> wrote: On 9/24/19 9:51 AM, Hohensee, Paul wrote: > I did JBS queries for, e.g., > > Issuetype=CSR AND Subcomponent="java.lang.management" AND fixVersion=11 > > For 11, I got > > https://bugs.openjdk.java.net/browse/JDK-8199295: Remove SNMP agent > https://bugs.openjdk.java.net/browse/JDK-8198732: ThreadInfo.from(CompositeData) does not throw IAE when the given CompositeData with missing attributes These two issues did not change jmm interface and no need to rev the version. Mandy > 8199295 seems to me to justify a minor version for 11. There weren't any management CSRs for 12 and 13 (I also checked for 'component="core-svc"'). CSRs don't exist for 8 and 9, but I checked > > Subcomponent="java.lang.management" AND fixVersion=<8 and 9 and their updates> > > I didn't find any spec changes for 8 and 9, so we would have > > JMM_VERSION_2_1 = 0x20020100, // JDK 11 > JMM_VERSION_2_2 = 0x20020200, // JDK 14 > JMM_VERSION = JMM_VERSION_2_2 > > Reasonable? > > On 9/24/19, 9:05 AM, "Daniel D. Daugherty" <daniel.daughe...@oracle.com> wrote: > > $ hg annot src/hotspot/share/include/jmm.h | grep JMM_VERSION_2 > 47592: JMM_VERSION_2 = 0x20020000, // JDK 10 > > $ hg log -r 47592 > changeset: 47592:68d46cb9be45 > user: uvangapally > date: Thu Oct 05 01:31:53 2017 -0700 > summary: 8185003: JMX: Add a version of ThreadMXBean.dumpAllThreads > with a maxDepth argument > > 8185003 was fixed in JDK10-B31. > > Dunno about the other releases... > > Dan > > > On 9/24/19 11:45 AM, Hohensee, Paul wrote: > > Good idea. The current definition is > > > > enum { > > JMM_VERSION_1 = 0x20010000, > > JMM_VERSION_1_0 = 0x20010000, > > JMM_VERSION_1_1 = 0x20010100, // JDK 6 > > JMM_VERSION_1_2 = 0x20010200, // JDK 7 > > JMM_VERSION_1_2_1 = 0x20010201, // JDK 7 GA > > JMM_VERSION_1_2_2 = 0x20010202, > > JMM_VERSION_2 = 0x20020000, // JDK 10 > > JMM_VERSION = 0x20020000 > > }; > > > > Were there changes in 11, 12, and 13 to justify adding major/minor versions for any of them? What was changed in 10 to justify a new major version? > > > > Absent any other additions, would this work? It creates a minor version for 14. > > > > JMM_VERSION_2_1 = 0x20020100, // JDK 14 > > JMM_VERSION = JMM_VERSION_2_1 > > > > Thanks, > > > > Paul > > > > On 9/23/19, 8:42 PM, "Mandy Chung" <mandy.ch...@oracle.com> wrote: > > > > Good question. > > > > When HS express (mix-n-matched JDK and HS version) was supported, the > > JMM_VERSION was rev'ed to enable the version checking. HS express is no > > longer supported. JDK is supported to run with this version of HotSpot > > VM. OTOH, this adds a new function in the middle of the function > > table. I think it's a good convention to follow and bump the version > > number. > > > > Mandy > > > > On 9/23/19 7:54 PM, Daniil Titov wrote: > > > Hi Paul, > > > > > > I have a question about JMM_VERSION. Since the changeset introduces a new method in the interface > > > should not JMM_VERSION declared in src/hotspot/share/include/jmm.h be bumped? > > > > > > Thank you, > > > --Daniil > > > > > > On 9/23/19, 5:43 PM, "serviceability-dev on behalf of Hohensee, Paul" <serviceability-dev-boun...@openjdk.java.net on behalf of hohen...@amazon.com> wrote: > > > > > > Update: > > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8231209 > > > CSR: https://bugs.openjdk.java.net/browse/JDK-8231374 > > > Webrev: http://cr.openjdk.java.net/~phh/8231209/webrev.01/ > > > > > > All test suites that reference getThreadAllocatedBytes pass. These are > > > > > > hotspot/jtreg/vmTestbase/nsk/monitoring (contained the failing test) > > > jdk/com/sun/management > > > jdk/jdk/jfr/event/runtime > > > > > > Per Mandy, the default getCurrentThreadAllocatedBytes implementation throws a UOE. > > > > > > The CSR is a copy of the original, and in addition points out that ThreadMXBean is a PlatformManagedObject, why that's important, and why a default getCurrentThreadAllocatedBytes implementation is necessary. > > > > > > I changed the nsk test to make sure that the approach it uses will work with getCurrentThreadAllocatedBytes, which per Mandy is defined as a property. Though I'm happy to remove it if there's a consensus it isn't needed. > > > > > > Thanks, > > > > > > Paul > > > > > > On 9/19/19, 11:03 PM, "serguei.spit...@oracle.com" <serguei.spit...@oracle.com> wrote: > > > > > > Hi Paul, > > > > > > I have almost the same comments as David: > > > - the same two spots of changes identified > > > - the addition of the default method was expected > > > - the change in test is a surprise (I also doubt, it is really needed) > > > - new CSR is needed > > > > > > > > > Sorry, I forgot to remind about running the vmTestbase monitoring tests. :( > > > > > > Thanks, > > > Serguei > > > > > > > > > On 9/19/19 16:06, David Holmes wrote: > > > > Hi Paul, > > > > > > > > On 20/09/2019 2:52 am, Hohensee, Paul wrote: > > > >> More formally, > > > >> > > > >> Bug: https://bugs.openjdk.java.net/browse/JDK-8231209 > > > >> Webrev: http://cr.openjdk.java.net/~phh/8231209/webrev.00/ > > > > > > > > I'm assuming there are only two changes here: > > > > > > > > 1. The new method is now a default method that throws UOE. > > > > > > > > That seems fine. > > > > > > > > 2. You implemented the new method in the test class. > > > > > > > > I don't understand why you did that. The test can't be calling the new > > > > method. Now that it is a default method we will get past the > > > > compilation failure that caused the problem. So no change to the test > > > > should be needed AFAICS. > > > > > > > > A new CSR request is needed. Just copy everything across from the old, > > > > with the updated spec. But please also mention this is a > > > > PlatformManagedObject in the compatibility discussion. > > > > > > > > Thanks, > > > > David > > > > > > > >> Thanks, > > > >> > > > >> On 9/19/19, 9:44 AM, "serviceability-dev on behalf of Hohensee, > > > >> Paul" <serviceability-dev-boun...@openjdk.java.net on behalf of > > > >> hohen...@amazon.com> wrote: > > > >> > > > >> Off by 2 error. Changed the subject to reflect 8231209. > > > >> http://cr.openjdk.java.net/~phh/8231209/webrev.00/ > > > >> Paul > > > >> On 9/19/19, 6:31 AM, "Daniel D. Daugherty" > > > >> <daniel.daughe...@oracle.com> wrote: > > > >> > http://cr.openjdk.java.net/~phh/8231211/webrev.00/ > > > >> The redo bug is 8231209. 8231211 is closed as a dup > > > >> of 8231210. > > > >> Dan > > > >> On 9/19/19 9:17 AM, Hohensee, Paul wrote: > > > >> > I'll have the default method throw UOE. That's the same as > > > >> the other default methods do. > > > >> > > > > >> > The necessary test fix is in > > > >> test/hotspot/jtreg/vmTestbase/nsk/monitoring/share/server/ServerThreadMXBeanNew.java, > > > >> which needs a new getCurrentThreadAllocatedBytes method, defined as > > > >> > > > > >> > public long getCurrentThreadAllocatedBytes() { > > > >> > return (Long) > > > >> invokeMethod("getCurrentThreadAllocatedBytes", > > > >> > new Object[] { }, > > > >> > new String[] { }); > > > >> > } > > > >> > > > > >> > With this fix, the 134 tests in > > > >> test/hotspot/jtreg/vmTestbase/nsk/monitoring/ThreadMXBean pass. > > > >> Preliminary webrev at > > > >> > > > > >> > http://cr.openjdk.java.net/~phh/8231211/webrev.00/ > > > >> > > > > >> > Is it worth adding getCurrentThreadAllocatedBytes tests to > > > >> the > > > >> test/hotspot/jtreg/vmTestbase/nsk/monitoring/ThreadMXBean/GetThreadAllocatedBytes > > > >> set? > > > >> > > > > >> > Paul > > > >> > > > > >> > On 9/18/19, 8:16 PM, "David Holmes" > > > >> <david.hol...@oracle.com> wrote: > > > >> > > > > >> > On 19/09/2019 12:57 pm, Mandy Chung wrote: > > > >> > > On 9/18/19 5:00 PM, Hohensee, Paul wrote: > > > >> > >> They all implement > > > >> com.sun.management.ThreadMXBean, so adding a > > > >> > >> getCurrentThreadAllocatedBytes broke them. > > > >> Potential fix is to give it > > > >> > >> a default implementation, vis > > > >> > >> > > > >> > >> public default long > > > >> getCurrentThreadAllocatedBytes() { > > > >> > >> return -1; > > > >> > >> } > > > >> > >> > > > >> > > > > > >> > > com.sun.management.ThreadMXBean (and other platform > > > >> MXBeans) is a > > > >> > > "sealed" interface which should only be implemented > > > >> by JDK. > > > >> > > > > >> > Didn't realize that. I don't recall knowing about > > > >> PlatformManagedObject. > > > >> > Sealed types will at least allow this to be enforced, > > > >> though I have to > > > >> > wonder what the tests are doing here. > > > >> > > > > >> > > Unfortunately we don't have the sealed type feature > > > >> yet. Yes it needs > > > >> > > to be a default method. I think it should throw UOE. > > > >> > > > > > >> > > * @implSpec > > > >> > > * The default implementation throws {@code > > > >> > > UnsupportedOperationException}. > > > >> > > > > > >> > > The @throw UOE can make it clear that it does not > > > >> support current thread > > > >> > > memory allocation measurement. > > > >> > > > > >> > Yes that seems a reasonable default if we don't want > > > >> this to be > > > >> > implemented outside the platform. > > > >> > > > > >> > Thanks, > > > >> > David > > > >> > > > > >> > > Mandy > > > >> > > > > >> > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >