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