Ping once more :)
Need a confirmatory review to push this. If I understand the rules correctly,
it doesn't need a Reviewer review since Mandy's already reviewed it, it just
needs a Committer review.
Thanks,
Paul
On 9/6/19, 11:08 AM, "hotspot-gc-dev on behalf of Hohensee, Paul"
<hotspot-gc-dev-boun...@openjdk.java.net on behalf of hohen...@amazon.com>
wrote:
Ping. Anyone? (
Thanks,
On 9/3/19, 12:39 PM, "serviceability-dev on behalf of Hohensee, Paul"
<serviceability-dev-boun...@openjdk.java.net on behalf of hohen...@amazon.com>
wrote:
Minor update in new webrev
http://cr.openjdk.java.net/~phh/8207266/webrev.05/. I removed
ensureNonNullThreadIds() in favor of Objects.requireNonNull(ids).
Thanks, Mandy, for your through reviews. May I get another reviewer to
weigh in?
Paul
On 8/31/19, 5:06 PM, "hotspot-gc-dev on behalf of Hohensee, Paul"
<hotspot-gc-dev-boun...@openjdk.java.net on behalf of hohen...@amazon.com>
wrote:
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
