RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
Mandy Chung
mandy.chung at oracle.com
Fri Aug 30 23:25:30 UTC 2019
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.chung at oracle.com>
> *Date: *Friday, August 30, 2019 at 10:22 AM
> *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
>
> 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>
> <mailto:mandy.chung at oracle.com>
> *Date: *Wednesday, August 28, 2019 at 3:59 PM
> *To: *"Hohensee, Paul" <hohensee at amazon.com>
> <mailto:hohensee at amazon.com>
> *Cc: *OpenJDK Serviceability <serviceability-dev at openjdk.java.net>
> <mailto:serviceability-dev at openjdk.java.net>,
> "hotspot-gc-dev at openjdk.java.net"
> <mailto:hotspot-gc-dev at openjdk.java.net>
> <hotspot-gc-dev at openjdk.java.net>
> <mailto: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
>
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190830/b8362d1d/attachment-0001.html>
More information about the serviceability-dev
mailing list