JDK-8231374 : [REDO] JDK-8207266 ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
  • Type: CSR
  • Component: core-svc
  • Sub-Component: java.lang.management
  • Priority: P4
  • Status: Closed
  • Resolution: Approved
  • Fix Versions: 14
  • Submitted: 2019-09-23
  • Updated: 2020-09-10
  • Resolved: 2019-10-23
Related Reports
CSR :  
Relates :  
Description
Summary
-------

Add `com.sun.management.ThreadMXBean::getCurrentThreadAllocatedBytes`.

Problem
-------

com.sun.management.ThreadMXBean includes getThreadAllocatedBytes in two versions, one which takes a thread_id and another which takes an array of thread_ids.

```
getThreadAllocatedBytes(long id);
getThreadAllocatedBytes(long[] ids);
```

It also includes versions of getThreadCpuTime and getThreadUserTime that take an array of thread_ids.

```
getThreadCpuTime(long[] ids);
getThreadUserTime(long[] ids);
```

java.lang.thread.ThreadMXBean defines single thread_id and current thread versions of these.

```
getThreadCpuTime(long id);
getThreadUserTime(long id);
getCurrentThreadCpuTime();
getCurrentThreadUserTime();
```

Three versions of Cpu and User time are thus available from com.sun.management.ThreadMXBean: current thread, single thread_id, and an array of thread_ids. Only two versions of AllocatedBytes are currently available: single thread_id and an array of thread_ids.

The current thread versions of Cpu and User time are optimized to by avoiding checks needed by the versions that take thread_id(s). getCurrentThreadAllocatedBytes can also be so optimized, is useful for applications that monitor themselves, and rounds out the set of AllocatedBytes methods. As with the other AllocatedBytes methods, getCurrentThreadAllocatedBytes is a method on com.sun.management.ThreadMXBean rather than java.lang.thread.ThreadMXBean in order to avoid requiring all implementations to support it.

Note also that c.s.m.ThreadMXBean extends j.l.m.ThreadMXBean, which in turn extends PlatformManagedObject. The Java 12 javadoc for the latter says

"The platform MXBean interfaces (i.e. all subinterfaces of PlatformManagedObject) are implemented by the Java platform only. New methods may be added in these interfaces in future Java SE releases. In addition, this PlatformManagedObject interface is only intended for the management interfaces for the platform to extend but not for applications."

In theory, this means that only platform (JDK library) code can extend c.s.m.ThreadMXBean, but until sealed types are implemented, it is still possible for user code to do so. In fact, the nsk ThreadMXBean monitoring tests do, hence getCurrentThreadAllocatedBytes must have a default implementation. Per its definition, that implementation is to throw an UnsupportedOperationException.

Solution
--------

Add `com.sun.management.ThreadMXBean::getCurrentThreadAllocatedBytes`.

Specification
-------------
```
/**
 * Returns an approximation of the total amount of memory, in bytes,
 * allocated in heap memory for the current thread.
 * The returned value is an approximation because some Java virtual machine
 * implementations may use object allocation mechanisms that result in a
 * delay between the time an object is allocated and the time its size is
 * recorded.
 *
 * <p>
 * This is a convenience method for local management use and is
 * equivalent to calling:
 * <blockquote><pre>
 *   {@link #getThreadAllocatedBytes getThreadAllocatedBytes}(Thread.currentThread().getId());
 * </pre></blockquote>
 *
 * @implSpec The default implementation throws
 * {@code UnsupportedOperationException}.
 *
 * @return an approximation of the total memory allocated, in bytes, in
 * heap memory for the current thread
 * if thread memory allocation measurement is enabled;
 * {@code -1} otherwise.
 *
 * @throws java.lang.UnsupportedOperationException if the Java virtual
 *         machine implementation does not support thread memory allocation
 *         measurement.
 *
 * @see #isThreadAllocatedMemorySupported
 * @see #isThreadAllocatedMemoryEnabled
 * @see #setThreadAllocatedMemoryEnabled
 * @since 14
 */
public default long getCurrentThreadAllocatedBytes() {
    throw new UnsupportedOperationException();
}
```

Comments
I accidentally touched "specification" section and then reverted my changes back.
09-07-2020

I accidentally changed "Compatibility Risk Description" section and then reverted my changes back.
08-07-2020

Fix-up CSR JDK-8232072. As a procedural matter, moving this CSR to Approved.
23-10-2019

I've reverted the Description to reflect what was pushed and filed JDK-8231968 to change the default implementation to getThreadAllocatedBytes(Thread.currentThread().getId()).
09-10-2019

There is only one implementation of c.s.m.ThreadMXBean - this is an implementation class of the JDK. And this interface is not for external implementation so we're really overthinking this one right now.
09-10-2019

As a reminder, the process is that a CSR must be in *approved* state before the corresponding bug fix is pushed, not "Provisional" or "Pended". On a procedural front, to clean this up [~phh], please 1) Update this CSR to match what was pushed 2) File another follow-up bug, which may have its own CSR to time, to track the remaining discussion on what should be done here technically. It is common and acceptable for new default methods to call existing methods on an interface. If the existing methods behave in an implementation, that is not default method's fault. My preference here would be to define the default method that implements the desired functionality using the existing methods. A specific implementation could override that functionality if desired. However, I don't strictly oppose throwing an exception in the default, but think it is unnecessarily harsh.
08-10-2019

An argument to leave things as-was (i.e., the default implementation of getCurrentThreadAllocatedBytes throws UOE) is to note that c.s.m.ThreadMXBean can be implemented in arbitrary ways, which means that getThreadAllocatedBytes may be implemented to have unpredictable and possibly surprising behavior. If the default implementation of getCurrentThreadAllocatedBytes is to call getThreadAllocatedBytes, then getCurrentThreadAllocatedBytes behavior will also be unpredictable and possibly surprising. The only way to guarantee meeting the specification we want, is to throw a UOE.
08-10-2019

I only just realised that JDK-8231209 was pushed without this CSR request being finalized and approved. Now we have a bit of a mess process wise. JDK-8231209 has been filed to change the implementation. So I suggest this CSR request be withdrawn and a new one created for JDK-8231209 with the desired content. With regard to the spec itself this should be quite simple and needs only state what the default implementation is. If everything else gets inherited then an @implNote may suffice.
07-10-2019

Going further, should we restrict the javadoc to saying that getCurrentThreadAllocatedBytes is equivalent to getThreadAllocatedBytes(Thread.currentThread().getId()) and leave out everything else? I don't know of a precedent for doing so though. getCurrentThreadCpu/UserTime don't do that. Perhaps they should, and a default implementation be provided for them too.
07-10-2019

I've updated the description and implSpec to say that the default implementation is to return the value of calling getThreadAllocatedBytes(Thread.currentThread().getId()). Since we're defining that as the default behavior, do we really want to say that a UOE is thrown if the default implementation is not overridden? Seems to me that saying that would constrain what getThreadAllocatedBytes() does.
07-10-2019

The method's spec could be written so an UOE was thrown if "Java virtual machine implementation does not support thread memory allocation measurement" OR the default was not overridden. However * @implSpec * This is a convenience method for local management use and is * equivalent to calling: * <blockquote><pre> * {@link #getThreadAllocatedBytes getThreadAllocatedBytes}(Thread.currentThread().getId()); * </pre></blockquote> seems preferable to me. I assume the existing methods would propagate a UOE from the existing methods.
03-10-2019

Further, I agreed with Mandy that the whole point of throwing the UOE was so that anyone incorrectly trying to extend this internal interface would be reminded of that fact and not aided in having a useful default.
03-10-2019

"Joe's suggestion" is what I suggested, after which Mandy suggested the UOE! Any default implementation must follow the specification for the new method: * equivalent to calling: * <blockquote><pre> * {@link #getThreadAllocatedBytes getThreadAllocatedBytes}(Thread.currentThread().getId()); * </pre></blockquote>
03-10-2019

Joe's suggestion is a good one. `this.getThreadAllocatedBytes(Thread.currentThread().getId())` is indeed a better default implementation while as noted above, this interface should only be implemented by JDK and have the proper implementation.
03-10-2019

[~dholmes] thanks for the review references. Throwing the UOE would be valid, but if this.getThreadAllocatedBytes(Thread.currentThread().getId()) were possible, that would strike me as more helpful. Perhaps it is not possible, hence my inquiry.
03-10-2019

Hi Joe, there was a discussion about that in the original review thread for 8207266 after it caused failures, and the CSR request was updated to reflect the discussion. Paul's original proposal is here: http://mail.openjdk.java.net/pipermail/serviceability-dev/2019-September/029179.html My suggestion is here: http://mail.openjdk.java.net/pipermail/serviceability-dev/2019-September/029185.html Mandy then proposed the UOE here: http://mail.openjdk.java.net/pipermail/serviceability-dev/2019-September/029200.html
03-10-2019

Marking as pended until discussion of the correct operations semantics of the default method is conducted.
03-10-2019

Possibly silly question: why isn't the implSpec of the default method this.getThreadAllocatedBytes(Thread.currentThread().getId())
01-10-2019

This new API in ThreadMXBean has no (minimal) incompatibility concern. IMO marking the compatibilty kind to source causes confusion.
24-09-2019

What should the compatibility kind be?
24-09-2019

[~mchung] The "compatibility kind" should be set appropriately - why did you clear it?
24-09-2019

Thanks you, David. I've finalized the CSR.
24-09-2019

Looks fine - Reviewed. Thanks
24-09-2019