Is there a tool that will generate a reversal patch?
On 9/18/19, 5:14 PM, "Daniel D. Daugherty" <daniel.daughe...@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-boun...@openjdk.java.net on behalf of hohen...@amazon.com> wrote:
>
> I'll take a look.
>
> On 9/18/19, 4:40 PM, "David Holmes" <david.hol...@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.spit...@oracle.com"
<serguei.spit...@oracle.com>
> >> *Date: *Tuesday, September 17, 2019 at 2:26 AM
> >> *To: *"Hohensee, Paul" <hohen...@amazon.com>, David Holmes
> >> <david.hol...@oracle.com>, Mandy Chung
<mandy.ch...@oracle.com>
> >> *Cc: *OpenJDK Serviceability
<serviceability-dev@openjdk.java.net>,
> >> "hotspot-gc-...@openjdk.java.net"
<hotspot-gc-...@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.spit...@oracle.com"
> >> <mailto:serguei.spit...@oracle.com>
<serguei.spit...@oracle.com>
> >> <mailto:serguei.spit...@oracle.com>
> >> *Organization: *Oracle Corporation
> >> *Date: *Friday, September 13, 2019 at 5:50 PM
> >> *To: *"Hohensee, Paul" <hohen...@amazon.com>
> >> <mailto:hohen...@amazon.com>, David Holmes
<david.hol...@oracle.com>
> >> <mailto:david.hol...@oracle.com>, Mandy Chung
> >> <mandy.ch...@oracle.com> <mailto:mandy.ch...@oracle.com>
> >> *Cc: *OpenJDK Serviceability
<serviceability-dev@openjdk.java.net>
> >> <mailto:serviceability-dev@openjdk.java.net>,
> >> "hotspot-gc-...@openjdk.java.net"
> >> <mailto:hotspot-gc-...@openjdk.java.net>
> >> <hotspot-gc-...@openjdk.java.net>
> >> <mailto:hotspot-gc-...@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.hol...@oracle.com>
> >> <mailto:david.hol...@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.ch...@oracle.com>
> >> <mailto:mandy.ch...@oracle.com>
> >>
> >> > *Date: *Thursday, September 12, 2019 at 10:09
AM
> >>
> >> > *To: *"Hohensee, Paul"<hohen...@amazon.com>
> >> <mailto:hohen...@amazon.com>
> >>
> >> > *Cc: *OpenJDK
> >> Serviceability<serviceability-dev@openjdk.java.net>
> >> <mailto:serviceability-dev@openjdk.java.net>,
> >>
> >> >"hotspot-gc-...@openjdk.java.net"
> >> <mailto:hotspot-gc-...@openjdk.java.net>
> >> <hotspot-gc-...@openjdk.java.net>
> >> <mailto:hotspot-gc-...@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
> >>
> >> >
> >>
> >>
> >>
> >>
> >>
>
>
>
>
