And I filed 8231211 for the same thing. :) Yes, please handle it, because it will go faster since I don't have access to a fast machine (just my laptop).
Webrev here: http://cr.openjdk.java.net/~phh/8231211/webrev.00/ Thanks, On 9/18/19, 5:25 PM, "Daniel D. Daugherty" <daniel.daughe...@oracle.com> wrote: I created this sub-task for you: JDK-8231210 [BACKOUT] JDK-8207266 ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread https://bugs.openjdk.java.net/browse/JDK-8231210 If you would prefer, I can handle this backout for you. Dan On 9/18/19 8:21 PM, Hohensee, Paul wrote: > Never having done this before, is it > > hg backout -r <original commit id> > > ? Do I file a JBS issue for the reversion? Seems necessary. > > On 9/18/19, 5:18 PM, "Daniel D. Daugherty" <daniel.daughe...@oracle.com> wrote: > > % hg backout > > is the usual way to do this... > > Dan > > > On 9/18/19 8:17 PM, Hohensee, Paul wrote: > > Is there a tool that will generate a reversal patch? > > > > On 9/18/19, 5:14 PM, "Daniel D. Daugherty" <daniel.daughe...@oracle.com> wrote: > > > > > Shall I go with that, or reverse the original patch? > > > > I'm a bit worried about what else might show up since the > > NSK monitoring tests were not run prior to this push. > > > > I vote for backing out the fix until proper testing has > > been done (and at least the one problem fixed...) > > > > Dan > > > > > > On 9/18/19 8:00 PM, Hohensee, Paul wrote: > > > 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 > > > >> > > > >> > > > > >> > > > >> > > > >> > > > >> > > > >> > > > > > > > > > > > > > > > > > > > > >
