Thanks David (and Mandy and Serguei). Pushed. On 9/17/19, 3:51 PM, "David Holmes" <david.hol...@oracle.com> wrote:
On 18/09/2019 12:10 am, Hohensee, Paul wrote: > Thanks, Serguei. :) > > David, are you ok with the patch? Yep, nothing further from me. David > 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 > > > > > > > > > >