+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" <david.hol...@oracle.com> 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"
>>> <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
>>> > > > >>
>>> > > > >> >
>>> > > > >>
>>> > > > >>
>>> > > > >>
>>> > > > >>
>>> > > > >>
>>> > > >
>>> > > >
>>> > > >
>>> > > >
>>> > >
>>> > >
>>> > >
>>> >
>>> >
>>> >
>>>
>>
>
