[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR (M): 8231209: [REDO] ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
From: "Hohensee, Paul" <hohensee () amazon ! com>
Date: 2019-09-25 21:23:45
Message-ID: 89E3E1E9-F0A6-431A-B931-32BDD6C73095 () amazon ! com
[Download RAW message or body]
Excellent!
David and Mandy have formally approved, so I'll push the current version of the patch.
Paul
On 9/25/19, 11:39 AM, "Mandy Chung" <mandy.chung@oracle.com> wrote:
One point to note is that JVM TI, JDI and JDWP are supported interfaces.
jmm.h (and jvm.h and possibly others) are private interface between VM
and libraries and it has the freedom to make any change (even dropping
JMM_VERSION - this can be done in the future).
For this patch to move forward, I have no objection if it's preferred to
use the same versioning scheme. Bumping JMM_VERSION for every release
is not necessary IMO.
Mandy
On 9/24/19 12:45 PM, serguei.spitsyn@oracle.com wrote:
> 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.chung@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.daugherty@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.chung@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@openjdk.java.net on behalf of
>> hohensee@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@oracle.com" <serguei.spitsyn@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@openjdk.java.net on behalf of
>> > > > >> hohensee@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@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@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
>> > > > >> >
>> > > > >> >
>> > > > >>
>> > > >
>> > > >
>> > > >
>> > > >
>> > > >
>> > > >
>> > > >
>> > >
>> > >
>> > >
>> >
>> >
>> >
>>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic