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