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