Thanks David (and Mandy and Serguei). Pushed.
On 9/17/19, 3:51 PM, "David Holmes" <[email protected]> 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: *"[email protected]" <[email protected]>
> *Date: *Tuesday, September 17, 2019 at 2:26 AM
> *To: *"Hohensee, Paul" <[email protected]>, David Holmes
> <[email protected]>, Mandy Chung <[email protected]>
> *Cc: *OpenJDK Serviceability <[email protected]>,
> "[email protected]" <[email protected]>
> *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: *"[email protected]"
> <mailto:[email protected]> <[email protected]>
> <mailto:[email protected]>
> *Organization: *Oracle Corporation
> *Date: *Friday, September 13, 2019 at 5:50 PM
> *To: *"Hohensee, Paul" <[email protected]>
> <mailto:[email protected]>, David Holmes <[email protected]>
> <mailto:[email protected]>, Mandy Chung
> <[email protected]> <mailto:[email protected]>
> *Cc: *OpenJDK Serviceability <[email protected]>
> <mailto:[email protected]>,
> "[email protected]"
> <mailto:[email protected]>
> <[email protected]>
> <mailto:[email protected]>
> *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"<[email protected]>
<mailto:[email protected]> 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<[email protected]>
<mailto:[email protected]>
>
> > *Date: *Thursday, September 12, 2019 at 10:09 AM
>
> > *To: *"Hohensee, Paul"<[email protected]>
<mailto:[email protected]>
>
> > *Cc: *OpenJDK
Serviceability<[email protected]>
<mailto:[email protected]>,
>
> >"[email protected]"
<mailto:[email protected]> <[email protected]>
<mailto:[email protected]>
>
> > *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
>
> >
>
>
>
>
>
>
>
