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>
*Date: *Thursday, September 12, 2019 at 10:09 AM
*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
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
