Paul
*From: *"serguei.spit...@oracle.com" <serguei.spit...@oracle.com>
*Date: *Tuesday, September 17, 2019 at 2:26 AM
*To: *"Hohensee, Paul" <hohen...@amazon.com>, David Holmes
<david.hol...@oracle.com>, Mandy Chung <mandy.ch...@oracle.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
Hi Paul,
Thank you for refactoring and fixing the test.
It looks great now!
Thanks,
Serguei
On 9/15/19 02:52, Hohensee, Paul wrote:
Hi, Serguei, thanks for the review. New webrev at
http://cr.openjdk.java.net/~phh/8207266/webrev.09/
I refactored the test’s main() method, and you’re correct,
getThreadAllocatedBytes should be getCurrentThreadAllocatedBytes in
that context: fixed.
Paul
*From: *"serguei.spit...@oracle.com"
<mailto:serguei.spit...@oracle.com> <serguei.spit...@oracle.com>
<mailto:serguei.spit...@oracle.com>
*Organization: *Oracle Corporation
*Date: *Friday, September 13, 2019 at 5:50 PM
*To: *"Hohensee, Paul" <hohen...@amazon.com>
<mailto:hohen...@amazon.com>, David Holmes <david.hol...@oracle.com>
<mailto:david.hol...@oracle.com>, Mandy Chung
<mandy.ch...@oracle.com> <mailto:mandy.ch...@oracle.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,
It looks pretty good in general.
http://cr.openjdk.java.net/~phh/8207266/webrev.08/test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemory.java.frames.html
It would be nice to refactor the java main() method as it becomes
too big.
Two ways ofgetCurrentThreadAllocatedBytes() testing are good
candidates
to become separate methods.
98 long size1 = mbean.getThreadAllocatedBytes(id);
Just wanted to double check if you wanted to invoke
the getCurrentThreadAllocatedBytes() instead as it is
a part of:
85 // First way, getCurrentThreadAllocatedBytes
Thanks,
Serguei
On 9/13/19 12:11 PM, Hohensee, Paul wrote:
Hi David, thanks for your comments. New webrev in
http://cr.openjdk.java.net/~phh/8207266/webrev.08/
Both the old and new versions of the code check that thread
allocated memory is both supported and enabled. The existing version
of getThreadAllocatedBytes(long []) calls
verifyThreadAllocatedMemory(long []), which checks inline to make sure
thread allocated memory is supported, then calls
isThreadAllocatedMemoryEnabled() to verify that it's enabled.
isThreadAllocatedMemoryEnabled() duplicates (!) the support check and
returns the enabled flag. I removed the redundant check in the new
version.
You're of course correct about the back-to-back check.
Application code can't know when the runtime will hijack a thread for
its own purposes. I've removed the check.
Paul
On 9/13/19, 12:50 AM, "David Holmes"<david.hol...@oracle.com>
<mailto:david.hol...@oracle.com> wrote:
Hi Paul,
On 13/09/2019 10:29 am, Hohensee, Paul wrote:
> Thanks for clarifying the review rules. Would someone
from the
> serviceability team please review? New webrev at
>
>http://cr.openjdk.java.net/~phh/8207266/webrev.07/
One aspect of the functional change needs clarification
for me - and
apologies if this has been covered in the past. It seems
to me that
currently we only check isThreadAllocatedMemorySupported
for these
operations, but if I read things correctly the updated
code additionally
checks isThreadAllocatedMemoryEnabled, which is a
behaviour change not
mentioned in the CSR.
> I didn’t disturb the existing checks in the test, just
added code to
> check the result of getThreadAllocatedBytes(long) on a
non-current
> thread, plus the back-to-back no-allocation checks. The
former wasn’t
> needed before because getThreadAllocatedBytes(long) was
just a wrapper
> around getThreadAllocatedBytes(long []). This patch
changes that, so I
> added a separate test. The latter is supposed to fail
if there’s object
> allocation on calls to getCurrentThreadAllocatedBytes and
> getThreadAllocatedBytes(long). I.e., a feature, not a
bug, because
> accumulation of transient small objects can be a
performance problem.
> Thanks to your review, I noticed that the back-to-back
check on the
> current thread was using getThreadAllocatedBytes(long)
instead of
> getCurrentThreadAllocatedBytes and fixed it. I also
removed all
> instances of “TEST FAILED: “.
The back-to-back check is not valid in general. You don't
know if the
first check might trigger some class loading on the
return path after it
has obtained the first memory value. The check might also
fail if using
JVMCI and some compilation related activity occurs in the
current thread
on the second call. Also with the introduction of
handshakes its
possible the current thread might hit a safepoint checks
that results in
it executing a handshake operation that performs
allocation. Potentially
there could be numerous non-deterministic actions that
might occur
leading to unanticipated allocation.
I understand what you want to test here, I just don't
think it is
reliably doable.
Thanks,
David
-----
>
> Paul
>
> *From: *Mandy Chung<mandy.ch...@oracle.com>
<mailto:mandy.ch...@oracle.com>
> *Date: *Thursday, September 12, 2019 at 10:09 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
>
> On 9/3/19 12:38 PM, Hohensee, Paul wrote:
>
> Minor update in new
webrevhttp://cr.openjdk.java.net/~phh/8207266/webrev.05/.
>
>
> I only reviewed the library side implementation that
looks good. I
> expect the serviceability team to review the test and
hotspot change.
>
>
> 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.
>
>
> You need another reviewer to advice the following
because I was not
> close to the ThreadsList work.
>
> 2087 ThreadsListHandle tlh;
>
> 2088 JavaThread* java_thread =
tlh.list()->find_JavaThread_from_java_tid(thread_id);
>
> 2089
>
> 2090 if (java_thread != NULL) {
>
> 2091 return java_thread->cooked_allocated_bytes();
>
> 2092 }
>
> This looks right to me.
>
>
test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemory.java
>
> - "ThreadAllocatedMemory is expected to
be disabled");
>
> + "TEST FAILED: ThreadAllocatedMemory is
expected to be
> disabled");
>
> Prepending "TEST FAILED" in exception message (in
several places)
>
> seems redundant since such RuntimeException is thrown
and expected
>
> a test failure.
>
> + // back-to-back calls shouldn't allocate any
memory
>
> + size = mbean.getThreadAllocatedBytes(id);
>
> + size1 = mbean.getThreadAllocatedBytes(id);
>
> + if (size1 != size) {
>
> Is there anything in the test can do to help guarantee
this? I didn't
>
> closely review this test. The main thing I advice is
to improve
>
> the reliability of this test. Put it in another way,
we want to
>
> ensure that this test change will pass all the time in
various
>
> test configuration.
>
> Mandy
>