RFR (M): 8231209: [REDO] ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread

[serguei.spitsyn at oracle.com]

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