RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread

Daniel D. Daugherty daniel.daugherty at oracle.com
Thu Sep 19 01:08:11 UTC 2019


We should probably repurpose

     JDK-8231209 Many com.sun.management.ThreadMXBean test failures 
after 8207266

as your REDO bug.

Dan


On 9/18/19 9:05 PM, Hohensee, Paul wrote:
> +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.holmes at 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.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 serviceability-dev mailing list