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

[David Holmes]

Hi Paul,

On 14/09/2019 5:11 am, 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.

Thanks for clarifying.

> 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.

Updated test looks fine.

Nothing further from me.

Thanks,
David
-----

> Paul
> 
> On 9/13/19, 12:50 AM, "David Holmes" <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>
>      > *Date: *Thursday, September 12, 2019 at 10:09 AM
>      > *To: *"Hohensee, Paul" <hohensee at amazon.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
>      >
>      > 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
>      >
>      
>