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/ 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: “. 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 webrev http://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
