They all implement com.sun.management.ThreadMXBean, so adding a getCurrentThreadAllocatedBytes broke them. Potential fix is to give it a default implementation, vis
public default long getCurrentThreadAllocatedBytes() { return -1; } Shall I go with that, or reverse the original patch? On 9/18/19, 4:48 PM, "serviceability-dev on behalf of Hohensee, Paul" <serviceability-dev-boun...@openjdk.java.net on behalf of hohen...@amazon.com> wrote: I'll take a look. On 9/18/19, 4:40 PM, "David Holmes" <david.hol...@oracle.com> wrote: Paul, Unfortunately this patch has broken the vmTestbase/nsk/monitoring tests: [2019-09-18T22:59:32,349Z] /scratch/mesos/jib-master/install/jdk-14+15-615/src.full/open/test/hotspot/jtreg/vmTestbase/nsk/monitoring/share/server/ServerThreadMXBeanNew.java:32: error: ServerThreadMXBeanNew is not abstract and does not override abstract method getCurrentThreadAllocatedBytes() in ThreadMXBean and possibly other issues as we are seeing hundreds of failures. David On 18/09/2019 8:50 am, David Holmes 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 >> >> > >> >> >> >> >>