$ 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
     >          >>          >
     >          >>          >
     >          >>
     >
     >
     >
     >
     >
     >
     >

Reply via email to