On 9/24/19 8:45 AM, Hohensee, Paul wrote:
Good idea. The current definition isenum { 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?
A new API ThreadMXBean.dumpAllThreads taking a maxDepth argument [1] was added in 10. It changed an existing function signature. I don't think any change was made to jmm.h since 10.
The JMM version was originally designed to follow the same convention as the old JDK versioning where major version revision indicates a major release with potential incompatible change whereas minor version revision indicates that no incompatible change to existing APIs. This explains why major version was bumped in 10.
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
Perhaps it's time to simplify the scheme to bump the major number when there is change in a JDK release.
JMM_VERSION_14 = 0x20040000, Mandy
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, theJMM_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 > >> > > >> > > >> > > > > > > >
