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

[Hohensee, Paul]

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