I used "hg backout -r 56298".
On 9/18/19, 5:46 PM, "Daniel D. Daugherty" <[email protected]> wrote:
For some reason, the backout that I did does not match the backout
that you did so I'm trying to figure that out.
Dan
On 9/18/19 8:36 PM, Hohensee, Paul wrote:
> And I filed 8231211 for the same thing. :)
>
> Yes, please handle it, because it will go faster since I don't have
access to a fast machine (just my laptop).
>
> Webrev here:
>
> http://cr.openjdk.java.net/~phh/8231211/webrev.00/
>
> Thanks,
>
> On 9/18/19, 5:25 PM, "Daniel D. Daugherty" <[email protected]>
wrote:
>
> I created this sub-task for you:
>
> JDK-8231210 [BACKOUT] JDK-8207266
> ThreadMXBean::getThreadAllocatedBytes() can be quicker for self
thread
> https://bugs.openjdk.java.net/browse/JDK-8231210
>
> If you would prefer, I can handle this backout for you.
>
> Dan
>
>
> On 9/18/19 8:21 PM, Hohensee, Paul wrote:
> > 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"
<[email protected]> 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"
<[email protected]> 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" <[email protected] on behalf of
[email protected]> wrote:
> > > >
> > > > I'll take a look.
> > > >
> > > > On 9/18/19, 4:40 PM, "David Holmes"
<[email protected]> 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: *"[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
> > > > >>
> > > > >> >
> > > > >>
> > > > >>
> > > > >>
> > > > >>
> > > > >>
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> >
> >
> >
>
>
>
