RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread

[Mandy Chung]

OK.  That's better.  Some review comments:

The javadoc of getCurrentThreadAllocatedBytes() can simply say:

"Returns an approximation of the total amount of memory, in bytes,
allocated in heap memory for the current thread.

This is a convenient method for local management use and is equivalent
to calling getThreadAllocatedBytes(Thread.currentThread().getId()).


src/hotspot/share/include/jmm.h

GetOneThreadsAllocatedMemory: s/OneThreads/OneThread/

sun/management/ThreadImpl.java

   43     private static final String 
THREAD_ALLOCATED_MEMORY_NOT_SUPPORTED =
   44         "Thread allocated memory measurement is not supported.";

if (!isThreadAllocatedMemorySupported()) {
    throw new 
UnsupportedOperationException(THREAD_ALLOCATED_MEMORY_NOT_SUPPORTED);
}

Perhaps the above can be refactored as 
throwIfAllocatedMemoryUnsupported() method.

  391         if (ids.length == 1) {
  392             sizes[0] = -1;
  :
  398             if (ids.length == 1) {
  399                 long id = ids[0];
  400                 sizes[0] = getThreadAllocatedMemory0(
  401                     Thread.currentThread().getId() == id ? 0 : id);
  402             } else {

It seems cleaner to handle the 1-element array case at the beginning
of this method:
    if (ids.length == 1) {
        long size = getThreadAllocatedBytes(ids[0]);
        return new long[] { size };
    }

I didn't review the hotspot implementation and the test.

Mandy

On 8/29/19 10:01 AM, Hohensee, Paul wrote:
>
> My bad, Mandy. The webrev puts getCurrentThreadAllocatedBytes in 
> com.sun.management.ThreadMXBean along with the current two 
> getThreadAllocatedBytes methods for the reasons you list. I’ve updated 
> the CSR to specify com.sun.management and added a rationale. 
> AllocatedBytes is currently enabled by Hotspot by default because the 
> overhead of recording TLAB occupancy is negligible.
>
> There’s no new GC code, nor will there be, so imo we don’t have to 
> involve the GC folks. I.e., the new JMM method 
> GetOneThreadsAllocatedBytes uses the existing cooked_allocated_bytes 
> JavaThread method, and getCurrentThreadAllocatedBytes is the same as 
> getThreadAllocatedBytes: it just bypasses the thread lookup code.
>
> I hadn’t tracked down what happens when getCurrentThreadUserTime and 
> getCurrentThreadCpuTime are called before, but if I’m not mistaken, it 
> the code in jcmd() in attachListener.cpp will call 
> GetThreadCpuTimeWithKind in management.cpp, and it will ultimately use 
> Thread::current() as the subject of the call, see 
> os::current_thread_cpu_time in os_linux.cpp. That means that the 
> CurrentThread methods should work remotely the same way they do 
> locally. GetOneThreadsAllocatedBytes in management.cpp uses THREAD as 
> its subject when called on behalf of getCurrentThreadAllocatedBytes, 
> so it will also uses the current remote Java thread. Even if these 
> methods only worked locally, there are many setups where apps are 
> self-monitoring that could use the performance improvement.
>
> Thanks,
>
> Paul
>
> *From: *Mandy Chung <mandy.chung at oracle.com>
> *Date: *Wednesday, August 28, 2019 at 3:59 PM
> *To: *"Hohensee, Paul" <hohensee at amazon.com>
> *Cc: *OpenJDK Serviceability <serviceability-dev at openjdk.java.net>, 
> "hotspot-gc-dev at openjdk.java.net" <hotspot-gc-dev at openjdk.java.net>
> *Subject: *Re: RFR (M): 8207266: 
> ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
>
> Hi Paul,
>
> The CSR proposes this method in java.lang.management.ThreadMXBean as a 
> Java SE feature.
>
> Has this been discussed with the GC team to commit measuring current 
> thread's allocated bytes as Java SE feature?   Can this be supported 
> by all JVM implementation?   What is the overhead if this is enabled 
> by default?  Does it need to be disabled?   This metric is from TLAB 
> that might be okay.  This needs advice/discussion with GC experts.
>
> I see that CSR mentions it can be disabled and link to 
> isThreadAllocatedMemoryEnabled() and setThreadAllocatedMemoryEnabled() 
> methods but these methods are defined in com.sun.management.ThreadMXBean.
>
> As Alan points out, current thread makes sense only in local VM 
> management.  When this is monitored from a JMX client (e.g. jconsole 
> to connect to a running JVM, "currentThreadAllowcatedBytes" attribute 
> is the current thread in jconsole process which invoking 
> Thread::currentThread?
>
> Mandy
>
> On 8/28/19 12:22 PM, Hohensee, Paul wrote:
>
>     Please review a performance improvement for
>     ThreadMXBean.getThreadAllocatedBytes and the addition of
>     getCurrentThreadAllocatedBytes.
>
>     JBS issue:https://bugs.openjdk.java.net/browse/JDK-8207266
>
>     Webrev:http://cr.openjdk.java.net/~phh/8207266/webrev.00/
>
>     CSR:https://bugs.openjdk.java.net/browse/JDK-8230311
>
>     Previous email threads:
>     https://mail.openjdk.java.net/pipermail/serviceability-dev/2018-July/024441.html
>     https://mail.openjdk.java.net/pipermail/serviceability-dev/2018-August/024763.html
>
>     The CSR is for adding ThreadMXBean.getCurrentThreadAllocatedBytes.
>     I’d be great for someone to review it.
>
>     I took Mandy’s advice and put the fast paths in the library code.
>     I added a new JMM method GetOneThreadsAllocatedBytes that works
>     the same as GetThreadCpuTime: it uses a thread_id value of zero to
>     distinguish the current thread. On my Mac laptop, the result runs
>     47x faster for the current thread than the old implementation.
>
>     The 3 tests intest/jdk/com/sun/management/ThreadMXBean all pass. I
>     added code to ThreadAllocatedMemory.java to test
>     getCurrentThreadAllocatedBytes as well as variations on
>     getThreadAllocatedBytes(id). A submit repo job is in progress.
>
>     Thanks,
>
>     Paul
>
>
>