+1, thanks! My apologies for the bad patch. I'll file another issue and run every test that mentions ThreadMXBean. At least, I know how to revert a patch now.
Paul On 9/18/19, 6:00 PM, "David Holmes" <[email protected]> wrote: Ship it! Thanks Dan! David On 19/09/2019 10:53 am, Daniel D. Daugherty wrote: > Looks like the issue is different versions of 'hg' in use. > > When I import Paul's patch from his webrev using my 'hg' and > then export it again, it matches my version of the backout. > > I have done a mechanical verification that the backout is an > exact reversal for Paul's original changeset. > > I'm planning to push the changeset with the following info: > > > 8231210: [BACKOUT] JDK-8207266 ThreadMXBean::getThreadAllocatedBytes() > can be quicker for self thread > Reviewed-by: phh, dholmes > > Everyone good with this? > > Dan > > On 9/18/19 8:44 PM, Daniel D. Daugherty wrote: >> For some reason, the backout that I did does not match the backout >> that you did so I'm trying to figure that out. >> >> Dan >> >> >> >> On 9/18/19 8:36 PM, Hohensee, Paul wrote: >>> 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" >>> <[email protected]> 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" >>> <[email protected]> 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" >>> <[email protected]> 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" >>> <[email protected] on behalf of >>> [email protected]> wrote: >>> > > > >>> > > > I'll take a look. >>> > > > >>> > > > On 9/18/19, 4:40 PM, "David Holmes" >>> <[email protected]> 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: *"[email protected]" >>> <[email protected]> >>> > > > >> *Date: *Tuesday, September 17, 2019 >>> at 2:26 AM >>> > > > >> *To: *"Hohensee, Paul" >>> <[email protected]>, David Holmes >>> > > > >> <[email protected]>, Mandy >>> Chung <[email protected]> >>> > > > >> *Cc: *OpenJDK Serviceability >>> <[email protected]>, >>> > > > >> "[email protected]" >>> <[email protected]> >>> > > > >> *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: >>> *"[email protected]" >>> > > > >> <mailto:[email protected]> >>> <[email protected]> >>> > > > >> <mailto:[email protected]> >>> > > > >> *Organization: *Oracle Corporation >>> > > > >> *Date: *Friday, September 13, >>> 2019 at 5:50 PM >>> > > > >> *To: *"Hohensee, Paul" >>> <[email protected]> >>> > > > >> <mailto:[email protected]>, David >>> Holmes <[email protected]> >>> > > > >> <mailto:[email protected]>, >>> Mandy Chung >>> > > > >> <[email protected]> >>> <mailto:[email protected]> >>> > > > >> *Cc: *OpenJDK Serviceability >>> <[email protected]> >>> > > > >> >>> <mailto:[email protected]>, >>> > > > >> "[email protected]" >>> > > > >> >>> <mailto:[email protected]> >>> > > > >> <[email protected]> >>> > > > >> >>> <mailto:[email protected]> >>> > > > >> *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"<[email protected]> >>> > > > >> <mailto:[email protected]> 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<[email protected]> >>> > > > >> <mailto:[email protected]> >>> > > > >> >>> > > > >> > *Date: *Thursday, September 12, >>> 2019 at 10:09 AM >>> > > > >> >>> > > > >> > *To: *"Hohensee, >>> Paul"<[email protected]> >>> > > > >> <mailto:[email protected]> >>> > > > >> >>> > > > >> > *Cc: *OpenJDK >>> > > > >> >>> Serviceability<[email protected]> >>> > > > >> >>> <mailto:[email protected]>, >>> > > > >> >>> > > > >> >"[email protected]" >>> > > > >> >>> <mailto:[email protected]> >>> > > > >> <[email protected]> >>> > > > >> >>> <mailto:[email protected]> >>> > > > >> >>> > > > >> > *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 >>> > > > >> >>> > > > >> > >>> > > > >> >>> > > > >> >>> > > > >> >>> > > > >> >>> > > > >> >>> > > > >>> > > > >>> > > > >>> > > > >>> > > >>> > > >>> > > >>> > >>> > >>> > >>> >> >
