Hi Paul,
That all seems fine to me.
CSR reviewed as well.
Thanks,
David
-----
On 24/09/2019 10:42 am, Hohensee, Paul 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
>> >
>> >
>>
