RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
Hohensee, Paul
hohensee at amazon.com
Thu Sep 19 00:55:26 UTC 2019
I am.
On 9/18/19, 5:54 PM, "Daniel D. Daugherty" <daniel.daugherty at oracle.com> 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.daugherty at 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.daugherty at 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.daugherty at 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-bounces at openjdk.java.net on behalf of
>> hohensee at amazon.com> wrote:
>> > > >
>> > > > I'll take a look.
>> > > >
>> > > > On 9/18/19, 4:40 PM, "David Holmes"
>> <david.holmes at 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.spitsyn at oracle.com"
>> <serguei.spitsyn at oracle.com>
>> > > > >> *Date: *Tuesday, September 17, 2019
>> at 2:26 AM
>> > > > >> *To: *"Hohensee, Paul"
>> <hohensee at amazon.com>, David Holmes
>> > > > >> <david.holmes at oracle.com>, Mandy
>> Chung <mandy.chung at oracle.com>
>> > > > >> *Cc: *OpenJDK Serviceability
>> <serviceability-dev at openjdk.java.net>,
>> > > > >> "hotspot-gc-dev at openjdk.java.net"
>> <hotspot-gc-dev at 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.spitsyn at oracle.com"
>> > > > >> <mailto:serguei.spitsyn at oracle.com>
>> <serguei.spitsyn at oracle.com>
>> > > > >> <mailto:serguei.spitsyn at oracle.com>
>> > > > >> *Organization: *Oracle Corporation
>> > > > >> *Date: *Friday, September 13,
>> 2019 at 5:50 PM
>> > > > >> *To: *"Hohensee, Paul"
>> <hohensee at amazon.com>
>> > > > >> <mailto:hohensee at amazon.com>, David
>> Holmes <david.holmes at oracle.com>
>> > > > >> <mailto:david.holmes at oracle.com>,
>> Mandy Chung
>> > > > >> <mandy.chung at oracle.com>
>> <mailto:mandy.chung at oracle.com>
>> > > > >> *Cc: *OpenJDK Serviceability
>> <serviceability-dev at openjdk.java.net>
>> > > > >>
>> <mailto:serviceability-dev at openjdk.java.net>,
>> > > > >> "hotspot-gc-dev at openjdk.java.net"
>> > > > >>
>> <mailto:hotspot-gc-dev at openjdk.java.net>
>> > > > >> <hotspot-gc-dev at openjdk.java.net>
>> > > > >>
>> <mailto:hotspot-gc-dev at 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.holmes at oracle.com>
>> > > > >> <mailto:david.holmes at 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.chung at oracle.com>
>> > > > >> <mailto:mandy.chung at oracle.com>
>> > > > >>
>> > > > >> > *Date: *Thursday, September 12,
>> 2019 at 10:09 AM
>> > > > >>
>> > > > >> > *To: *"Hohensee,
>> Paul"<hohensee at amazon.com>
>> > > > >> <mailto:hohensee at amazon.com>
>> > > > >>
>> > > > >> > *Cc: *OpenJDK
>> > > > >>
>> Serviceability<serviceability-dev at openjdk.java.net>
>> > > > >>
>> <mailto:serviceability-dev at openjdk.java.net>,
>> > > > >>
>> > > > >> >"hotspot-gc-dev at openjdk.java.net"
>> > > > >>
>> <mailto:hotspot-gc-dev at openjdk.java.net>
>> > > > >> <hotspot-gc-dev at openjdk.java.net>
>> > > > >>
>> <mailto:hotspot-gc-dev at 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
>> > > > >>
>> > > > >> >
>> > > > >>
>> > > > >>
>> > > > >>
>> > > > >>
>> > > > >>
>> > > >
>> > > >
>> > > >
>> > > >
>> > >
>> > >
>> > >
>> >
>> >
>> >
>>
>
More information about the hotspot-gc-dev
mailing list