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

Hohensee, Paul hohensee at amazon.com
Thu Sep 19 01:09:14 UTC 2019


Good idea, will do.

On 9/18/19, 6:08 PM, "Daniel D. Daugherty" <daniel.daugherty at oracle.com> wrote:

    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