RFR (M): 8231209: [REDO] ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
Mandy Chung
mandy.chung at oracle.com
Tue Sep 24 16:38:21 UTC 2019
On 9/24/19 8: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?
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.chung at 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-bounces at openjdk.java.net on behalf of hohensee at 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.spitsyn at oracle.com" <serguei.spitsyn at 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-bounces at openjdk.java.net on behalf of
> > >> hohensee at 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.daugherty at 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.holmes at 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
> > >> >
> > >> >
> > >>
> >
> >
> >
> >
> >
> >
> >
>
>
>
More information about the serviceability-dev
mailing list