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

David Holmes david.holmes at oracle.com
Tue Sep 17 22:50:24 UTC 2019


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