Hi Paul and Mandy,

There was a move from Mikael Vidsted to unify the versioning.
The idea is to keep versions in sync with the JDK versions.
Now, for JVMTI we have (see, https://bugs.openjdk.java.net/browse/JDK-8219023):

enum {
    JVMTI_VERSION_1   = 0x30010000,
    JVMTI_VERSION_1_0 = 0x30010000,
    JVMTI_VERSION_1_1 = 0x30010100,
    JVMTI_VERSION_1_2 = 0x30010200,
    JVMTI_VERSION_9   = 0x30090000,
    JVMTI_VERSION_11  = 0x300B0000,

    JVMTI_VERSION = 0x30000000 + (14 * 0x10000) + ( 0 * 0x100) + 0 /* version: 14.0.0 */
};

Also, it was decided to auto bump the JVMTI and JDWP versions
for new releases no matter the spec was changed or not.

I've added Mikael to the cc-list.

Thanks,
Serguei


On 9/24/19 9:58 AM, Hohensee, Paul wrote:
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
     >      >      >          >>          >
     >      >      >          >>          >
     >      >      >          >>
     >      >      >
     >      >      >
     >      >      >
     >      >      >
     >      >      >
     >      >      >
     >      >      >
     >      >
     >      >
     >      >
     >
     >
     >

Reply via email to