Thanks, Mandy. I’ve finalized the CSR. New webrev at 
http://cr.openjdk.java.net/~phh/8207266/webrev.04/.

In management.cpp, I now have

    if (THREAD->is_Java_thread()) {
      return ((JavaThread*)THREAD)->cooked_allocated_bytes();
    }


In ThreadImpl.java, using requireNonNull would produce a different and less 
informative message, so I’d like to leave it as is. I changed 
throwIfNullThreadIds to ensureNonNullThreadIds, and 
throwIfThreadAllocatedMemoryNotSupported to 
ensureThreadAllocatedMemorySupported.



I dropped the “java.lang.” prefix from all uses of 
UnsupportedOperationException in both c.s.m.ThreadMXBean.java and 
j.l.m.ThreadMXBean.java, and did the same with SecurityException.



“@since 14” added to c.s.m.ThreadMXBean.java and the CSR.



Do I need another reviewer?



Paul

From: Mandy Chung <mandy.ch...@oracle.com>
Date: Friday, August 30, 2019 at 4:26 PM
To: "Hohensee, Paul" <hohen...@amazon.com>
Cc: OpenJDK Serviceability <serviceability-dev@openjdk.java.net>, 
"hotspot-gc-...@openjdk.java.net" <hotspot-gc-...@openjdk.java.net>
Subject: Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be 
quicker for self thread

CSR reviewed.

management.cpp
2083     java_thread = (JavaThread*)THREAD;
2084     if (java_thread->is_Java_thread()) {
2085       return java_thread->cooked_allocated_bytes();
2086     }

The cast should be done after is_Java_thread() test.

ThreadImpl.java
 162     private void throwIfNullThreadIds(long[] ids) {

Even better: simply use Objects::requiresNonNull and this method can be removed.

This suggests positive naming alternative to 
throwIfThreadAllocatedMemoryNotSupported - 
"ensureThreadAllocatedMemorySupported" (sorry I should have suggested that)

ThreadMXBean.java
 130      * @throws java.lang.UnsupportedOperationException if the Java virtual

Nit: "java.lang." can be dropped.

@since 14 is missing.

Mandy
On 8/30/19 3:33 PM, Hohensee, Paul wrote:
Thanks for your review, Mandy. Revised webrev at 
http://cr.openjdk.java.net/~phh/8207266/webrev.02/.

I updated the CSR with your suggested javadoc for 
getCurrentThreadAllocatedBytes. It now matches that for 
getCurrentThreadUserTime and getCurrentThreadCputime. I also fixed the 
“convenient” -> “convenience” typos in j.l.m.ThreadMXBean.java.

I meant GetOneThreads to be the possessive, but don’t feel strongly either way 
so I’m fine with GetOneThread.


I updated ThreadImpl.java as you suggested, though in 
getThreadAllocatedBytes(long[] ids) I had to add a 
redundant-in-the-not-length-1-case check for a null ids reference.



Would someone take a look at the Hotspot side and the test please?



Paul

From: Mandy Chung <mandy.ch...@oracle.com><mailto:mandy.ch...@oracle.com>
Date: Friday, August 30, 2019 at 10:22 AM
To: "Hohensee, Paul" <hohen...@amazon.com><mailto:hohen...@amazon.com>
Cc: OpenJDK Serviceability 
<serviceability-dev@openjdk.java.net><mailto:serviceability-dev@openjdk.java.net>,
 "hotspot-gc-...@openjdk.java.net"<mailto:hotspot-gc-...@openjdk.java.net> 
<hotspot-gc-...@openjdk.java.net><mailto:hotspot-gc-...@openjdk.java.net>
Subject: Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be 
quicker for self thread

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.ch...@oracle.com><mailto:mandy.ch...@oracle.com>
Date: Wednesday, August 28, 2019 at 3:59 PM
To: "Hohensee, Paul" <hohen...@amazon.com><mailto:hohen...@amazon.com>
Cc: OpenJDK Serviceability 
<serviceability-dev@openjdk.java.net><mailto:serviceability-dev@openjdk.java.net>,
 "hotspot-gc-...@openjdk.java.net"<mailto:hotspot-gc-...@openjdk.java.net> 
<hotspot-gc-...@openjdk.java.net><mailto:hotspot-gc-...@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 in test/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










Reply via email to