Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
> http://cr.openjdk.java.net/~phh/8231211/webrev.00/
The redo bug is 8231209. 8231211 is closed as a dup of 8231210.
Dan
On 9/19/19 9:17 AM, Hohensee, Paul wrote:
I'll have the default method throw UOE. That's the same as the other default
methods do.
The necessary test fix is in
test/hotspot/jtreg/vmTestbase/nsk/monitoring/share/server/ServerThreadMXBeanNew.java,
which needs a new getCurrentThreadAllocatedBytes method, defined as
public long getCurrentThreadAllocatedBytes() {
return (Long) invokeMethod("getCurrentThreadAllocatedBytes",
new Object[] { },
new String[] { });
}
With this fix, the 134 tests in
test/hotspot/jtreg/vmTestbase/nsk/monitoring/ThreadMXBean pass. Preliminary
webrev at
http://cr.openjdk.java.net/~phh/8231211/webrev.00/
Is it worth adding getCurrentThreadAllocatedBytes tests to the
test/hotspot/jtreg/vmTestbase/nsk/monitoring/ThreadMXBean/GetThreadAllocatedBytes
set?
Paul
On 9/18/19, 8:16 PM, "David Holmes" wrote:
On 19/09/2019 12:57 pm, Mandy Chung wrote:
> On 9/18/19 5: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;
>> }
>>
>
> com.sun.management.ThreadMXBean (and other platform MXBeans) is a
> "sealed" interface which should only be implemented by JDK.
Didn't realize that. I don't recall knowing about PlatformManagedObject.
Sealed types will at least allow this to be enforced, though I have to
wonder what the tests are doing here.
> Unfortunately we don't have the sealed type feature yet. Yes it needs
> to be a default method. I think it should throw UOE.
>
> * @implSpec
> * The default implementation throws {@code
> UnsupportedOperationException}.
>
> The @throw UOE can make it clear that it does not support current thread
> memory allocation measurement.
Yes that seems a reasonable default if we don't want this to be
implemented outside the platform.
Thanks,
David
> Mandy
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
I'll have the default method throw UOE. That's the same as the other default
methods do.
The necessary test fix is in
test/hotspot/jtreg/vmTestbase/nsk/monitoring/share/server/ServerThreadMXBeanNew.java,
which needs a new getCurrentThreadAllocatedBytes method, defined as
public long getCurrentThreadAllocatedBytes() {
return (Long) invokeMethod("getCurrentThreadAllocatedBytes",
new Object[] { },
new String[] { });
}
With this fix, the 134 tests in
test/hotspot/jtreg/vmTestbase/nsk/monitoring/ThreadMXBean pass. Preliminary
webrev at
http://cr.openjdk.java.net/~phh/8231211/webrev.00/
Is it worth adding getCurrentThreadAllocatedBytes tests to the
test/hotspot/jtreg/vmTestbase/nsk/monitoring/ThreadMXBean/GetThreadAllocatedBytes
set?
Paul
On 9/18/19, 8:16 PM, "David Holmes" wrote:
On 19/09/2019 12:57 pm, Mandy Chung wrote:
> On 9/18/19 5: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;
>> }
>>
>
> com.sun.management.ThreadMXBean (and other platform MXBeans) is a
> "sealed" interface which should only be implemented by JDK.
Didn't realize that. I don't recall knowing about PlatformManagedObject.
Sealed types will at least allow this to be enforced, though I have to
wonder what the tests are doing here.
> Unfortunately we don't have the sealed type feature yet. Yes it needs
> to be a default method. I think it should throw UOE.
>
> * @implSpec
> * The default implementation throws {@code
> UnsupportedOperationException}.
>
> The @throw UOE can make it clear that it does not support current thread
> memory allocation measurement.
Yes that seems a reasonable default if we don't want this to be
implemented outside the platform.
Thanks,
David
> Mandy
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
On 19/09/2019 12:57 pm, Mandy Chung wrote:
On 9/18/19 5: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;
}
com.sun.management.ThreadMXBean (and other platform MXBeans) is a
"sealed" interface which should only be implemented by JDK.
Didn't realize that. I don't recall knowing about PlatformManagedObject.
Sealed types will at least allow this to be enforced, though I have to
wonder what the tests are doing here.
Unfortunately we don't have the sealed type feature yet. Yes it needs
to be a default method. I think it should throw UOE.
* @implSpec
* The default implementation throws {@code
UnsupportedOperationException}.
The @throw UOE can make it clear that it does not support current thread
memory allocation measurement.
Yes that seems a reasonable default if we don't want this to be
implemented outside the platform.
Thanks,
David
Mandy
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
On 9/18/19 5: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;
}
com.sun.management.ThreadMXBean (and other platform MXBeans) is a
"sealed" interface which should only be implemented by JDK.
Unfortunately we don't have the sealed type feature yet. Yes it needs
to be a default method. I think it should throw UOE.
* @implSpec
* The default implementation throws {@code
UnsupportedOperationException}.
The @throw UOE can make it clear that it does not support current thread
memory allocation measurement.
Mandy
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
his 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"
> >>> >>> hohen...@amazon.com> wrote:
> >>> > > >
> >>> > > > I'll take a look.
> >>> > > >
> >>> > > > On 9/18/19, 4:40 PM, "David Holmes"
> >>> 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"
> >>>
> >>> > > > >> *Date: *Tuesday, September 17,
2019
> >>> at 2:26 AM
> >>> > &g
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
> > > 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"
>>> >> hohen...@amazon.com> wrote:
>>> > > >
>>> > > > I'll take a look.
>>> > > >
>>> > > > On 9/18/19, 4:40 PM, "David Holmes"
>>> 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"
>>>
>>> > > > >> *Date: *Tuesday, September 17, 2019
>>> at 2:26 AM
>>> > > > >> *To: *"Hohensee, Paul"
>>> , David Holmes
>>> > > > >> , Mandy
>>> Chung
>>> > > > >> *Cc: *OpenJDK Serviceability
>>> ,
>>> > > > >> "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:
>>> &
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
ch?
>>> > > >
>>> > > > On 9/18/19, 4:48 PM, "serviceability-dev on
>>> behalf of Hohensee, Paul"
>>> >> hohen...@amazon.com> wrote:
>>> > > >
>>> > > > I'll take a look.
>>> > > >
>>> > > > On 9/18/19, 4:40 PM, "David Holmes"
>>> 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"
>>>
>>> > > > >> *Date: *Tuesday, September 17, 2019
>>> at 2:26 AM
>>> > > > >> *To: *"Hohensee, Paul"
>>> , David Holmes
>>> > > > >> , Mandy
>>> Chung
>>> > > > >> *Cc: *OpenJDK Serviceability
>>> ,
>>> > > > >> "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
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
> > > > > > > Yep, nothing further from me. > > > > > > > > David > > > > > > > >> Paul > > > >> > > > >> *From: *"serguei.spit...@oracle.com" > > > >> *Date: *Tuesday, September 17, 2019 at 2:26 AM > > > >> *To: *"Hohensee, Paul" , David Holmes > > > >> , Mandy Chung > > > >> *Cc: *OpenJDK Serviceability , > > > >> "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> > > > >> <mailto:serguei.spit...@oracle.com> > > > >> *Organization: *Oracle Corporation > > > >> *Date: *Friday, September 13, 2019 at 5:50 PM > > > >> *To: *"Hohensee, Paul" > > > >> <mailto:hohen...@amazon.com>, David Holmes > > > >> <mailto:david.hol...@oracle.com>, Mandy Chung > > > >> <mailto:mandy.ch...@oracle.com> > > > >> *Cc: *OpenJDK Serviceability > > > >> <mailto:serviceability-dev@openjdk.java.net>, > > > >> "hotspot-gc-...@openjdk.java.net" > > > >> <mailto: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. > > > >>
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
> 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"
>>
>> > > > >> *Date: *Tuesday, September 17, 2019
>> at 2:26 AM
>> > > > >> *To: *"Hohensee, Paul"
>> , David Holmes
>> > > > >> , Mandy
>> Chung
>> > > > >> *Cc: *OpenJDK Serviceability
>> ,
>> > > > >> "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>
>>
>> > > > >> <mailto:serguei.spit..
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
> > >> Thanks, Serguei. :)
> > > > >>
> > > > >> David, are you ok with the patch?
> > > > >
> > > > > Yep, nothing further from me.
> > > > >
> > > > > David
> > > > >
> > > > >> Paul
> > > > >>
> > > > >> *From: *"serguei.spit...@oracle.com"
> > > > >> *Date: *Tuesday, September 17, 2019 at
2:26 AM
> > > > >> *To: *"Hohensee, Paul"
, David Holmes
> > > > >> , Mandy Chung
> > > > >> *Cc: *OpenJDK Serviceability
,
> > > > >> "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>
> > > > >> <mailto:serguei.spit...@oracle.com>
> > > > >> *Organization: *Oracle Corporation
> > > > >> *Date: *Friday, September 13, 2019
at 5:50 PM
> > > > >> *To: *"Hohensee, Paul"
> > > > >> <mailto:hohen...@amazon.com>, David
Holmes
> > > > >> <mailto:david.hol...@oracle.com>,
Mandy Chung
> > > > >>
<mailto:mandy.ch...@oracle.com>
> > > > >> *Cc: *OpenJDK Serviceability
> > > > >>
<mailto:serviceability-dev@openjdk.java.net>,
> > > > >> "hotspot-gc-...@openjdk.java.net"
> > > > >>
<mailto:hotspot-gc-...@openjdk.java.net>
> > > > >>
> > > > >>
<mailto:hotspot-gc-...@openjdk.java.net>
> > > > >> *Subject: *Re: RFR (M): 8207266:
> > > > >>
ThreadMXBean::getThreadAllocatedBytes() can be quicker for self
> > > > >> thread
> > > >
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
; Yep, nothing further from me. > > > > > > > > David > > > > > > > >> Paul > > > >> > > > >> *From: *"serguei.spit...@oracle.com" > > > >> *Date: *Tuesday, September 17, 2019 at 2:26 AM > > > >> *To: *"Hohensee, Paul" , David Holmes > > > >> , Mandy Chung > > > >> *Cc: *OpenJDK Serviceability , > > > >> "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> > > > >> <mailto:serguei.spit...@oracle.com> > > > >> *Organization: *Oracle Corporation > > > >> *Date: *Friday, September 13, 2019 at 5:50 PM > > > >> *To: *"Hohensee, Paul" > > > >> <mailto:hohen...@amazon.com>, David Holmes > > > >> <mailto:david.hol...@oracle.com>, Mandy Chung > > > >> <mailto:mandy.ch...@oracle.com> > > > >> *Cc: *OpenJDK Serviceability > > > >> <mailto:serviceability-dev@openjdk.java.net>, > > > >> "hotspot-gc-...@openjdk.java.net" > > > >> <mailto: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 > >
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
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" 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
>
> ? Do I file a JBS issue for the reversion? Seems necessary.
>
> On 9/18/19, 5:18 PM, "Daniel D. Daugherty"
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"
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" wrote:
> > >
> > > I'll take a look.
> > >
> > > On 9/18/19, 4:40 PM, "David Holmes"
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"
> > > >> *Date: *Tuesday, September 17, 2019 at 2:26 AM
> > > >> *To: *"Hohensee, Paul" ,
David Holmes
> > >
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
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" 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
>
> ? Do I file a JBS issue for the reversion? Seems necessary.
>
> On 9/18/19, 5:18 PM, "Daniel D. Daugherty"
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"
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" wrote:
> > >
> > > I'll take a look.
> > >
> > > On 9/18/19, 4:40 PM, "David Holmes"
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"
> > > >> *Date: *Tuesday, September 17, 2019 at 2:26 AM
> > > >> *To: *"Hohensee, Paul" ,
David Holmes
> > > >> , Mandy Chung
> > > >> *Cc: *OpenJDK Serviceability
,
> > > >> "hotspot-gc-...@openjdk.java.net"
> > > &g
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
I have the backout ready to go. About to send out a webrev...
Dan
On 9/18/19 8:25 PM, Daniel D. Daugherty 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
? Do I file a JBS issue for the reversion? Seems necessary.
On 9/18/19, 5:18 PM, "Daniel D. Daugherty"
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"
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" behalf of hohen...@amazon.com> wrote:
> >
> > I'll take a look.
> >
> > On 9/18/19, 4:40 PM, "David Holmes"
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"
> > >> *Date: *Tuesday, September 17, 2019 at 2:26 AM
> > >> *To: *"Hohensee, Paul"
, David Holmes
> > >> , Mandy Chung
> > >> *Cc: *OpenJDK Serviceability
,
> > >> "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.n
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
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
? Do I file a JBS issue for the reversion? Seems necessary.
On 9/18/19, 5:18 PM, "Daniel D. Daugherty" 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"
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"
wrote:
> >
> > I'll take a look.
> >
> > On 9/18/19, 4:40 PM, "David Holmes"
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"
> > >> *Date: *Tuesday, September 17, 2019 at 2:26 AM
> > >> *To: *"Hohensee, Paul" , David
Holmes
> > >> , Mandy Chung
> > >> *Cc: *OpenJDK Serviceability
,
> > >> "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
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
Hi Paul,
On 19/09/2019 10:00 am, 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?
Any default implementation must follow the specification for the new method:
* equivalent to calling:
*
* {@link #getThreadAllocatedBytes
getThreadAllocatedBytes}(Thread.currentThread().getId());
*
The CSR request for this seems to be quite misleading in the
compatibility section:
"Adds a method with well-understood semantics and leaves existing method
semantics unchanged. Existing programs will continue to compile and
execute as before. "
It may have been assumed, based on the wording, that there would be a
default implementation, otherwise the second sentence is obviously not
correct. But it's clear from the specification section that this was not
going to be a default method.
It may be better to back this out and redo with a revised CSR request.
Thanks,
David
On 9/18/19, 4:48 PM, "serviceability-dev on behalf of Hohensee, Paul"
wrote:
I'll take a look.
On 9/18/19, 4:40 PM, "David Holmes" 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"
>> *Date: *Tuesday, September 17, 2019 at 2:26 AM
>> *To: *"Hohensee, Paul" , David Holmes
>> , Mandy Chung
>> *Cc: *OpenJDK Serviceability ,
>> "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>
>> <mailto:serguei.spit...@oracle.com>
>> *Organization: *Oracle Corporation
>> *Date: *Friday, September 13, 2019 at 5:50 PM
>> *To: *"Hohensee, Paul"
>> <mailto:hohen...@amazon.com>, David Holmes
>> <mailto:david.hol...@oracle.com>, Mandy Chung
>> <mailto:mandy.ch...@oracle.com>
>> *Cc: *OpenJDK Serviceability
>> <mailto:serviceability-dev@openjdk.java.net>,
>> "hotspot-gc-...@openjdk.java.net"
>> <mailto: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/ma
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
Never having done this before, is it
hg backout -r
? Do I file a JBS issue for the reversion? Seems necessary.
On 9/18/19, 5:18 PM, "Daniel D. Daugherty" 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"
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" wrote:
> >
> > I'll take a look.
> >
> > On 9/18/19, 4:40 PM, "David Holmes"
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"
> > >> *Date: *Tuesday, September 17, 2019 at 2:26 AM
> > >> *To: *"Hohensee, Paul" , David
Holmes
> > >> , Mandy Chung
> > >> *Cc: *OpenJDK Serviceability
,
> > >> "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
> > >>
> >
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
% 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" 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"
wrote:
>
> I'll take a look.
>
> On 9/18/19, 4:40 PM, "David Holmes" 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"
> >> *Date: *Tuesday, September 17, 2019 at 2:26 AM
> >> *To: *"Hohensee, Paul" , David Holmes
> >> , Mandy Chung
> >> *Cc: *OpenJDK Serviceability
,
> >> "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>
> >> <mailto:serguei.spit...@oracle.com>
> >> *Organization: *Oracle Corporation
> >> *Date: *Friday, September 13, 2019 at 5:50 PM
> >> *To: *"Hohensee, Paul"
> >> <mailto:hohen...@amazon.com>, David Holmes
> >> <mailto:david.hol...@oracle.com>, Mandy Chung
> >> <mailto:mandy.ch...@oracle.com>
> >> *Cc: *OpenJDK Serviceability
> >> <mailto:serviceability-dev@openjdk.java.net>,
> >> "hotspot-gc-...@openjdk.java.net"
> >> <mailto:hotspot-gc-...@openjdk.java.net>
> >>
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
Is there a tool that will generate a reversal patch?
On 9/18/19, 5:14 PM, "Daniel D. Daugherty" 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"
wrote:
>
> I'll take a look.
>
> On 9/18/19, 4:40 PM, "David Holmes" 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"
> >> *Date: *Tuesday, September 17, 2019 at 2:26 AM
> >> *To: *"Hohensee, Paul" , David Holmes
> >> , Mandy Chung
> >> *Cc: *OpenJDK Serviceability
,
> >> "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>
> >> <mailto:serguei.spit...@oracle.com>
> >> *Organization: *Oracle Corporation
> >> *Date: *Friday, September 13, 2019 at 5:50 PM
> >> *To: *"Hohensee, Paul"
> >> <mailto:hohen...@amazon.com>, David Holmes
> >> <mailto:david.hol...@oracle.com>, Mandy Chung
> >> <mailto:mandy.ch...@oracle.com>
> >> *Cc: *OpenJDK Serviceability
> >> <mailto:serviceability-dev@openjdk.java.net>,
> >> "hotspot-gc-...@openjdk.java.net"
> >> <mailto:hotspot-gc-...@openjdk.java.net>
> >>
> >> <mailto:hotspot-gc-...@openjdk.java.net>
> >> *Subject: *Re: RFR (M)
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
> 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"
wrote:
I'll take a look.
On 9/18/19, 4:40 PM, "David Holmes" 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"
>> *Date: *Tuesday, September 17, 2019 at 2:26 AM
>> *To: *"Hohensee, Paul" , David Holmes
>> , Mandy Chung
>> *Cc: *OpenJDK Serviceability ,
>> "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>
>> <mailto:serguei.spit...@oracle.com>
>> *Organization: *Oracle Corporation
>> *Date: *Friday, September 13, 2019 at 5:50 PM
>> *To: *"Hohensee, Paul"
>> <mailto:hohen...@amazon.com>, David Holmes
>> <mailto:david.hol...@oracle.com>, Mandy Chung
>> <mailto:mandy.ch...@oracle.com>
>> *Cc: *OpenJDK Serviceability
>> <mailto:serviceability-dev@openjdk.java.net>,
>> "hotspot-gc-...@openjdk.java.net"
>> <mailto: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 in
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
I've filed https://bugs.openjdk.java.net/browse/JDK-8231209 for this quick fix.
A better fix is to support getCurrentThreadAllocatedBytes in these tests.
On 9/18/19, 5:02 PM, "hotspot-gc-dev on behalf of 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"
wrote:
I'll take a look.
On 9/18/19, 4:40 PM, "David Holmes" 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"
>> *Date: *Tuesday, September 17, 2019 at 2:26 AM
>> *To: *"Hohensee, Paul" , David Holmes
>> , Mandy Chung
>> *Cc: *OpenJDK Serviceability
,
>> "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>
>> <mailto:serguei.spit...@oracle.com>
>> *Organization: *Oracle Corporation
>> *Date: *Friday, September 13, 2019 at 5:50 PM
>> *To: *"Hohensee, Paul"
>> <mailto:hohen...@amazon.com>, David Holmes
>> <mailto:david.hol...@oracle.com>, Mandy Chung
>> <mailto:mandy.ch...@oracle.com>
>> *Cc: *OpenJDK Serviceability
>> <mailto:serviceability-dev@openjdk.java.net>,
>> "hotspot-gc-...@openjdk.java.net"
>> <mailto: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
>> candi
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
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"
wrote:
I'll take a look.
On 9/18/19, 4:40 PM, "David Holmes" 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"
>> *Date: *Tuesday, September 17, 2019 at 2:26 AM
>> *To: *"Hohensee, Paul" , David Holmes
>> , Mandy Chung
>> *Cc: *OpenJDK Serviceability ,
>> "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>
>> <mailto:serguei.spit...@oracle.com>
>> *Organization: *Oracle Corporation
>> *Date: *Friday, September 13, 2019 at 5:50 PM
>> *To: *"Hohensee, Paul"
>> <mailto:hohen...@amazon.com>, David Holmes
>> <mailto:david.hol...@oracle.com>, Mandy Chung
>> <mailto:mandy.ch...@oracle.com>
>> *Cc: *OpenJDK Serviceability
>> <mailto:serviceability-dev@openjdk.java.net>,
>> "hotspot-gc-...@openjdk.java.net"
>> <mailto: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:
>>
>>
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
I'll take a look.
On 9/18/19, 4:40 PM, "David Holmes" 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"
>> *Date: *Tuesday, September 17, 2019 at 2:26 AM
>> *To: *"Hohensee, Paul" , David Holmes
>> , Mandy Chung
>> *Cc: *OpenJDK Serviceability ,
>> "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>
>> <mailto:serguei.spit...@oracle.com>
>> *Organization: *Oracle Corporation
>> *Date: *Friday, September 13, 2019 at 5:50 PM
>> *To: *"Hohensee, Paul"
>> <mailto:hohen...@amazon.com>, David Holmes
>> <mailto:david.hol...@oracle.com>, Mandy Chung
>> <mailto:mandy.ch...@oracle.com>
>> *Cc: *OpenJDK Serviceability
>> <mailto:serviceability-dev@openjdk.java.net>,
>> "hotspot-gc-...@openjdk.java.net"
>> <mailto: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
>> versi
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
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" *Date: *Tuesday, September 17, 2019 at 2:26 AM *To: *"Hohensee, Paul" , David Holmes , Mandy Chung *Cc: *OpenJDK Serviceability , "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> <mailto:serguei.spit...@oracle.com> *Organization: *Oracle Corporation *Date: *Friday, September 13, 2019 at 5:50 PM *To: *"Hohensee, Paul" <mailto:hohen...@amazon.com>, David Holmes <mailto:david.hol...@oracle.com>, Mandy Chung <mailto:mandy.ch...@oracle.com> *Cc: *OpenJDK Serviceability <mailto:serviceability-dev@openjdk.java.net>, "hotspot-gc-...@openjdk.java.net" <mailto: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" <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 getThread
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
Thanks David (and Mandy and Serguei). Pushed.
On 9/17/19, 3:51 PM, "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"
> *Date: *Tuesday, September 17, 2019 at 2:26 AM
> *To: *"Hohensee, Paul" , David Holmes
> , Mandy Chung
> *Cc: *OpenJDK Serviceability ,
> "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>
> <mailto:serguei.spit...@oracle.com>
> *Organization: *Oracle Corporation
> *Date: *Friday, September 13, 2019 at 5:50 PM
> *To: *"Hohensee, Paul"
> <mailto:hohen...@amazon.com>, David Holmes
> <mailto:david.hol...@oracle.com>, Mandy Chung
> <mailto:mandy.ch...@oracle.com>
> *Cc: *OpenJDK Serviceability
> <mailto:serviceability-dev@openjdk.java.net>,
> "hotspot-gc-...@openjdk.java.net"
> <mailto: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"
<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/
>
>
>
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
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"
*Date: *Tuesday, September 17, 2019 at 2:26 AM
*To: *"Hohensee, Paul" , David Holmes
, Mandy Chung
*Cc: *OpenJDK Serviceability ,
"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>
<mailto:serguei.spit...@oracle.com>
*Organization: *Oracle Corporation
*Date: *Friday, September 13, 2019 at 5:50 PM
*To: *"Hohensee, Paul"
<mailto:hohen...@amazon.com>, David Holmes
<mailto:david.hol...@oracle.com>, Mandy Chung
<mailto:mandy.ch...@oracle.com>
*Cc: *OpenJDK Serviceability
<mailto:serviceability-dev@openjdk.java.net>,
"hotspot-gc-...@openjdk.java.net"
<mailto: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"
<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
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"
Organization: Oracle Corporation
Date: Friday, September 13, 2019 at 5:50 PM
To: "Hohensee, Paul" ,
David Holmes , Mandy Chung
Cc: OpenJDK Serviceability
,
"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 of
getCurrentThreadAllocatedBytes() 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" 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 “TES
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" 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 > *Date: *Thursday, September 12, 2019 at 10:09 AM > *To: *"Hohensee, Paul" > *Cc: *OpenJDK Serviceability , > "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 anot
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
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" 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
> *Date: *Thursday, September 12, 2019 at 10:09 AM
> *To: *"Hohensee, Paul"
> *Cc: *OpenJDK Serviceability ,
> "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.
&
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
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" 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
> *Date: *Thursday, September 12, 2019 at 10:09 AM
> *To: *"Hohensee, Paul"
> *Cc: *OpenJDK Serviceability ,
> "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");
>
> +
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
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
*Date: *Thursday, September 12, 2019 at 10:09 AM
*To: *"Hohensee, Paul"
*Cc: *OpenJDK Serviceability ,
"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
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
And of course the back-to-back check is more or less accurate depending on the
accuracy of the underlying Hotspot mechanism. So it’s possible (indeed likely
with the current TLAB refill interval update) that it’ll return false
negatives, but imo better to keep it anyway.
From: "Hohensee, Paul"
Date: Thursday, September 12, 2019 at 5:29 PM
To: Mandy Chung
Cc: OpenJDK Serviceability ,
"hotspot-gc-...@openjdk.java.net"
Subject: Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be
quicker for self thread
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/
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: “.
Paul
From: Mandy Chung
Date: Thursday, September 12, 2019 at 10:09 AM
To: "Hohensee, Paul"
Cc: OpenJDK Serviceability ,
"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 webrev http://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
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
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/
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: “.
Paul
From: Mandy Chung
Date: Thursday, September 12, 2019 at 10:09 AM
To: "Hohensee, Paul"
Cc: OpenJDK Serviceability ,
"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 webrev http://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
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 webrev http://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
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
Ping once more :)
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.
Thanks,
Paul
On 9/6/19, 11:08 AM, "hotspot-gc-dev on behalf of Hohensee, Paul"
wrote:
Ping. Anyone? (
Thanks,
On 9/3/19, 12:39 PM, "serviceability-dev on behalf of Hohensee, Paul"
wrote:
Minor update in new webrev
http://cr.openjdk.java.net/~phh/8207266/webrev.05/. I removed
ensureNonNullThreadIds() in favor of Objects.requireNonNull(ids).
Thanks, Mandy, for your through reviews. May I get another reviewer to
weigh in?
Paul
On 8/31/19, 5:06 PM, "hotspot-gc-dev on behalf of Hohensee, Paul"
wrote:
Thanks, Mandy. I’ve finalized the CSR. New webrev at
http://cr.openjdk.java.net/~phh/8207266/webrev.04/.
In management.cpp, I now have
if (THREAD->is_Java_thread()) {
return ((JavaThread*)THREAD)->cooked_allocated_bytes();
}
In ThreadImpl.java, using requireNonNull would produce a different
and less informative message, so I’d like to leave it as is. I changed
throwIfNullThreadIds to ensureNonNullThreadIds, and
throwIfThreadAllocatedMemoryNotSupported to
ensureThreadAllocatedMemorySupported.
I dropped the “java.lang.” prefix from all uses of
UnsupportedOperationException in both c.s.m.ThreadMXBean.java and
j.l.m.ThreadMXBean.java, and did the same with SecurityException.
“@since 14” added to c.s.m.ThreadMXBean.java and the CSR.
Do I need another reviewer?
Paul
From: Mandy Chung
Date: Friday, August 30, 2019 at 4:26 PM
To: "Hohensee, Paul"
Cc: OpenJDK Serviceability ,
"hotspot-gc-...@openjdk.java.net"
Subject: Re: RFR (M): 8207266:
ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
CSR reviewed.
management.cpp
2083 java_thread = (JavaThread*)THREAD;
2084 if (java_thread->is_Java_thread()) {
2085 return java_thread->cooked_allocated_bytes();
2086 }
The cast should be done after is_Java_thread() test.
ThreadImpl.java
162 private void throwIfNullThreadIds(long[] ids) {
Even better: simply use Objects::requiresNonNull and this method
can be removed.
This suggests positive naming alternative to
throwIfThreadAllocatedMemoryNotSupported -
"ensureThreadAllocatedMemorySupported" (sorry I should have suggested that)
ThreadMXBean.java
130 * @throws java.lang.UnsupportedOperationException if the
Java virtual
Nit: "java.lang." can be dropped.
@since 14 is missing.
Mandy
On 8/30/19 3:33 PM, Hohensee, Paul wrote:
Thanks for your review, Mandy. Revised webrev at
http://cr.openjdk.java.net/~phh/8207266/webrev.02/.
I updated the CSR with your suggested javadoc for
getCurrentThreadAllocatedBytes. It now matches that for
getCurrentThreadUserTime and getCurrentThreadCputime. I also fixed the
“convenient” -> “convenience” typos in j.l.m.ThreadMXBean.java.
I meant GetOneThreads to be the possessive, but don’t feel strongly
either way so I’m fine with GetOneThread.
I updated ThreadImpl.java as you suggested, though in
getThreadAllocatedBytes(long[] ids) I had to add a
redundant-in-the-not-length-1-case check for a null ids reference.
Would someone take a look at the Hotspot side and the test please?
Paul
From: Mandy Chung
<mailto:mandy.ch...@oracle.com>
Date: Friday, August 30, 2019 at 10:22 AM
To: "Hohensee, Paul"
<mailto:hohen...@amazon.com>
Cc: OpenJDK Serviceability
<mailto:serviceability-dev@openjdk.java.net>,
"hotspot-gc-...@openjdk.java.net"<mailto:hotspot-gc-...@openjdk.java.net>
<mailto:hotspot-gc-...@openjdk.java.net>
Subject: Re: RFR (M): 8207266:
ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
OK. That's better. Some review comments:
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
Ping. Anyone? (
Thanks,
On 9/3/19, 12:39 PM, "serviceability-dev on behalf of Hohensee, Paul"
wrote:
Minor update in new webrev
http://cr.openjdk.java.net/~phh/8207266/webrev.05/. I removed
ensureNonNullThreadIds() in favor of Objects.requireNonNull(ids).
Thanks, Mandy, for your through reviews. May I get another reviewer to
weigh in?
Paul
On 8/31/19, 5:06 PM, "hotspot-gc-dev on behalf of Hohensee, Paul"
wrote:
Thanks, Mandy. I’ve finalized the CSR. New webrev at
http://cr.openjdk.java.net/~phh/8207266/webrev.04/.
In management.cpp, I now have
if (THREAD->is_Java_thread()) {
return ((JavaThread*)THREAD)->cooked_allocated_bytes();
}
In ThreadImpl.java, using requireNonNull would produce a different and
less informative message, so I’d like to leave it as is. I changed
throwIfNullThreadIds to ensureNonNullThreadIds, and
throwIfThreadAllocatedMemoryNotSupported to
ensureThreadAllocatedMemorySupported.
I dropped the “java.lang.” prefix from all uses of
UnsupportedOperationException in both c.s.m.ThreadMXBean.java and
j.l.m.ThreadMXBean.java, and did the same with SecurityException.
“@since 14” added to c.s.m.ThreadMXBean.java and the CSR.
Do I need another reviewer?
Paul
From: Mandy Chung
Date: Friday, August 30, 2019 at 4:26 PM
To: "Hohensee, Paul"
Cc: OpenJDK Serviceability ,
"hotspot-gc-...@openjdk.java.net"
Subject: Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes()
can be quicker for self thread
CSR reviewed.
management.cpp
2083 java_thread = (JavaThread*)THREAD;
2084 if (java_thread->is_Java_thread()) {
2085 return java_thread->cooked_allocated_bytes();
2086 }
The cast should be done after is_Java_thread() test.
ThreadImpl.java
162 private void throwIfNullThreadIds(long[] ids) {
Even better: simply use Objects::requiresNonNull and this method can be
removed.
This suggests positive naming alternative to
throwIfThreadAllocatedMemoryNotSupported -
"ensureThreadAllocatedMemorySupported" (sorry I should have suggested that)
ThreadMXBean.java
130 * @throws java.lang.UnsupportedOperationException if the Java
virtual
Nit: "java.lang." can be dropped.
@since 14 is missing.
Mandy
On 8/30/19 3:33 PM, Hohensee, Paul wrote:
Thanks for your review, Mandy. Revised webrev at
http://cr.openjdk.java.net/~phh/8207266/webrev.02/.
I updated the CSR with your suggested javadoc for
getCurrentThreadAllocatedBytes. It now matches that for
getCurrentThreadUserTime and getCurrentThreadCputime. I also fixed the
“convenient” -> “convenience” typos in j.l.m.ThreadMXBean.java.
I meant GetOneThreads to be the possessive, but don’t feel strongly
either way so I’m fine with GetOneThread.
I updated ThreadImpl.java as you suggested, though in
getThreadAllocatedBytes(long[] ids) I had to add a
redundant-in-the-not-length-1-case check for a null ids reference.
Would someone take a look at the Hotspot side and the test please?
Paul
From: Mandy Chung
<mailto:mandy.ch...@oracle.com>
Date: Friday, August 30, 2019 at 10:22 AM
To: "Hohensee, Paul" <mailto:hohen...@amazon.com>
Cc: OpenJDK Serviceability
<mailto:serviceability-dev@openjdk.java.net>,
"hotspot-gc-...@openjdk.java.net"<mailto:hotspot-gc-...@openjdk.java.net>
<mailto:hotspot-gc-...@openjdk.java.net>
Subject: Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes()
can be quicker for self thread
OK. That's better. Some review comments:
The javadoc of getCurrentThreadAllocatedBytes() can simply say:
"Returns an approximation of the total amount of memory, in bytes,
allocated in heap memory for the current thread.
This is a convenient method for local management use and is equivalent
to calling getThreadAllocatedBytes(Thread.currentThread().getId()).
src/hotspot/share/include/jmm.h
GetOneThreadsAllocatedMemory: s/OneThreads/OneThread/
sun/management/ThreadImpl.java
43 private static final String
THREAD_ALLOCATED_MEMORY_NOT_SUPPORTED =
44 &q
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
Minor update in new webrev http://cr.openjdk.java.net/~phh/8207266/webrev.05/.
I removed ensureNonNullThreadIds() in favor of Objects.requireNonNull(ids).
Thanks, Mandy, for your through reviews. May I get another reviewer to weigh in?
Paul
On 8/31/19, 5:06 PM, "hotspot-gc-dev on behalf of Hohensee, Paul"
wrote:
Thanks, Mandy. I’ve finalized the CSR. New webrev at
http://cr.openjdk.java.net/~phh/8207266/webrev.04/.
In management.cpp, I now have
if (THREAD->is_Java_thread()) {
return ((JavaThread*)THREAD)->cooked_allocated_bytes();
}
In ThreadImpl.java, using requireNonNull would produce a different and less
informative message, so I’d like to leave it as is. I changed
throwIfNullThreadIds to ensureNonNullThreadIds, and
throwIfThreadAllocatedMemoryNotSupported to
ensureThreadAllocatedMemorySupported.
I dropped the “java.lang.” prefix from all uses of
UnsupportedOperationException in both c.s.m.ThreadMXBean.java and
j.l.m.ThreadMXBean.java, and did the same with SecurityException.
“@since 14” added to c.s.m.ThreadMXBean.java and the CSR.
Do I need another reviewer?
Paul
From: Mandy Chung
Date: Friday, August 30, 2019 at 4:26 PM
To: "Hohensee, Paul"
Cc: OpenJDK Serviceability ,
"hotspot-gc-...@openjdk.java.net"
Subject: Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can
be quicker for self thread
CSR reviewed.
management.cpp
2083 java_thread = (JavaThread*)THREAD;
2084 if (java_thread->is_Java_thread()) {
2085 return java_thread->cooked_allocated_bytes();
2086 }
The cast should be done after is_Java_thread() test.
ThreadImpl.java
162 private void throwIfNullThreadIds(long[] ids) {
Even better: simply use Objects::requiresNonNull and this method can be
removed.
This suggests positive naming alternative to
throwIfThreadAllocatedMemoryNotSupported -
"ensureThreadAllocatedMemorySupported" (sorry I should have suggested that)
ThreadMXBean.java
130 * @throws java.lang.UnsupportedOperationException if the Java
virtual
Nit: "java.lang." can be dropped.
@since 14 is missing.
Mandy
On 8/30/19 3:33 PM, Hohensee, Paul wrote:
Thanks for your review, Mandy. Revised webrev at
http://cr.openjdk.java.net/~phh/8207266/webrev.02/.
I updated the CSR with your suggested javadoc for
getCurrentThreadAllocatedBytes. It now matches that for
getCurrentThreadUserTime and getCurrentThreadCputime. I also fixed the
“convenient” -> “convenience” typos in j.l.m.ThreadMXBean.java.
I meant GetOneThreads to be the possessive, but don’t feel strongly either
way so I’m fine with GetOneThread.
I updated ThreadImpl.java as you suggested, though in
getThreadAllocatedBytes(long[] ids) I had to add a
redundant-in-the-not-length-1-case check for a null ids reference.
Would someone take a look at the Hotspot side and the test please?
Paul
From: Mandy Chung <mailto:mandy.ch...@oracle.com>
Date: Friday, August 30, 2019 at 10:22 AM
To: "Hohensee, Paul" <mailto:hohen...@amazon.com>
Cc: OpenJDK Serviceability
<mailto:serviceability-dev@openjdk.java.net>,
"hotspot-gc-...@openjdk.java.net"<mailto:hotspot-gc-...@openjdk.java.net>
<mailto:hotspot-gc-...@openjdk.java.net>
Subject: Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can
be quicker for self thread
OK. That's better. Some review comments:
The javadoc of getCurrentThreadAllocatedBytes() can simply say:
"Returns an approximation of the total amount of memory, in bytes,
allocated in heap memory for the current thread.
This is a convenient method for local management use and is equivalent
to calling getThreadAllocatedBytes(Thread.currentThread().getId()).
src/hotspot/share/include/jmm.h
GetOneThreadsAllocatedMemory: s/OneThreads/OneThread/
sun/management/ThreadImpl.java
43 private static final String THREAD_ALLOCATED_MEMORY_NOT_SUPPORTED =
44 "Thread allocated memory measurement is not supported.";
if (!isThreadAllocatedMemorySupported()) {
throw new
UnsupportedOperationException(THREAD_ALLOCATED_MEMORY_NOT_SUPPORTED);
}
Perhaps the above can be refactored as throwIfAllocatedMemoryUnsupported()
method.
391 if (ids.length == 1) {
392 sizes[0] = -1;
:
398 if (ids.length == 1) {
399 long id = ids[0];
400 sizes[0] = getThreadAllocatedMemory0(
401 Thread.cur
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
Thanks, Mandy. I’ve finalized the CSR. New webrev at
http://cr.openjdk.java.net/~phh/8207266/webrev.04/.
In management.cpp, I now have
if (THREAD->is_Java_thread()) {
return ((JavaThread*)THREAD)->cooked_allocated_bytes();
}
In ThreadImpl.java, using requireNonNull would produce a different and less
informative message, so I’d like to leave it as is. I changed
throwIfNullThreadIds to ensureNonNullThreadIds, and
throwIfThreadAllocatedMemoryNotSupported to
ensureThreadAllocatedMemorySupported.
I dropped the “java.lang.” prefix from all uses of
UnsupportedOperationException in both c.s.m.ThreadMXBean.java and
j.l.m.ThreadMXBean.java, and did the same with SecurityException.
“@since 14” added to c.s.m.ThreadMXBean.java and the CSR.
Do I need another reviewer?
Paul
From: Mandy Chung
Date: Friday, August 30, 2019 at 4:26 PM
To: "Hohensee, Paul"
Cc: OpenJDK Serviceability ,
"hotspot-gc-...@openjdk.java.net"
Subject: Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be
quicker for self thread
CSR reviewed.
management.cpp
2083 java_thread = (JavaThread*)THREAD;
2084 if (java_thread->is_Java_thread()) {
2085 return java_thread->cooked_allocated_bytes();
2086 }
The cast should be done after is_Java_thread() test.
ThreadImpl.java
162 private void throwIfNullThreadIds(long[] ids) {
Even better: simply use Objects::requiresNonNull and this method can be removed.
This suggests positive naming alternative to
throwIfThreadAllocatedMemoryNotSupported -
"ensureThreadAllocatedMemorySupported" (sorry I should have suggested that)
ThreadMXBean.java
130 * @throws java.lang.UnsupportedOperationException if the Java virtual
Nit: "java.lang." can be dropped.
@since 14 is missing.
Mandy
On 8/30/19 3:33 PM, Hohensee, Paul wrote:
Thanks for your review, Mandy. Revised webrev at
http://cr.openjdk.java.net/~phh/8207266/webrev.02/.
I updated the CSR with your suggested javadoc for
getCurrentThreadAllocatedBytes. It now matches that for
getCurrentThreadUserTime and getCurrentThreadCputime. I also fixed the
“convenient” -> “convenience” typos in j.l.m.ThreadMXBean.java.
I meant GetOneThreads to be the possessive, but don’t feel strongly either way
so I’m fine with GetOneThread.
I updated ThreadImpl.java as you suggested, though in
getThreadAllocatedBytes(long[] ids) I had to add a
redundant-in-the-not-length-1-case check for a null ids reference.
Would someone take a look at the Hotspot side and the test please?
Paul
From: Mandy Chung <mailto:mandy.ch...@oracle.com>
Date: Friday, August 30, 2019 at 10:22 AM
To: "Hohensee, Paul" <mailto:hohen...@amazon.com>
Cc: OpenJDK Serviceability
<mailto:serviceability-dev@openjdk.java.net>,
"hotspot-gc-...@openjdk.java.net"<mailto:hotspot-gc-...@openjdk.java.net>
<mailto:hotspot-gc-...@openjdk.java.net>
Subject: Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be
quicker for self thread
OK. That's better. Some review comments:
The javadoc of getCurrentThreadAllocatedBytes() can simply say:
"Returns an approximation of the total amount of memory, in bytes,
allocated in heap memory for the current thread.
This is a convenient method for local management use and is equivalent
to calling getThreadAllocatedBytes(Thread.currentThread().getId()).
src/hotspot/share/include/jmm.h
GetOneThreadsAllocatedMemory: s/OneThreads/OneThread/
sun/management/ThreadImpl.java
43 private static final String THREAD_ALLOCATED_MEMORY_NOT_SUPPORTED =
44 "Thread allocated memory measurement is not supported.";
if (!isThreadAllocatedMemorySupported()) {
throw new
UnsupportedOperationException(THREAD_ALLOCATED_MEMORY_NOT_SUPPORTED);
}
Perhaps the above can be refactored as throwIfAllocatedMemoryUnsupported()
method.
391 if (ids.length == 1) {
392 sizes[0] = -1;
:
398 if (ids.length == 1) {
399 long id = ids[0];
400 sizes[0] = getThreadAllocatedMemory0(
401 Thread.currentThread().getId() == id ? 0 : id);
402 } else {
It seems cleaner to handle the 1-element array case at the beginning
of this method:
if (ids.length == 1) {
long size = getThreadAllocatedBytes(ids[0]);
return new long[] { size };
}
I didn't review the hotspot implementation and the test.
Mandy
On 8/29/19 10:01 AM, Hohensee, Paul wrote:
My bad, Mandy. The webrev puts getCurrentThreadAllocatedBytes in
com.sun.management.ThreadMXBean along with the current two
getThreadAllocatedBytes methods for the reasons you list. I’ve updated the CSR
to specify com.sun.management and added a rationale. AllocatedBytes is
currently enabled by Hotspot by default because the overhead of recording TLAB
occupancy is negligible.
There’s no new GC code, nor wil
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
CSR reviewed.
management.cpp
2083 java_thread = (JavaThread*)THREAD;
2084 if (java_thread->is_Java_thread()) {
2085 return java_thread->cooked_allocated_bytes();
2086 }
The cast should be done after is_Java_thread() test.
ThreadImpl.java
162 private void throwIfNullThreadIds(long[] ids) {
Even better: simply use Objects::requiresNonNull and this method can be
removed.
This suggests positive naming alternative to
throwIfThreadAllocatedMemoryNotSupported -
"ensureThreadAllocatedMemorySupported" (sorry I should have suggested that)
ThreadMXBean.java
130 * @throws java.lang.UnsupportedOperationException if the Java
virtual
Nit: "java.lang." can be dropped.
@since 14 is missing.
Mandy
On 8/30/19 3:33 PM, Hohensee, Paul wrote:
Thanks for your review, Mandy. Revised webrev at
http://cr.openjdk.java.net/~phh/8207266/webrev.02/.
I updated the CSR with your suggested javadoc for
getCurrentThreadAllocatedBytes. It now matches that for
getCurrentThreadUserTime and getCurrentThreadCputime. I also fixed the
“convenient” -> “convenience” typos in j.l.m.ThreadMXBean.java.
I meant GetOneThreads to be the possessive, but don’t feel strongly
either way so I’m fine with GetOneThread.
I updated ThreadImpl.java as you suggested, though in
getThreadAllocatedBytes(long[] ids) I had to add a
redundant-in-the-not-length-1-case check for a null ids reference.
Would someone take a look at the Hotspot side and the test please?
Paul
*From: *Mandy Chung
*Date: *Friday, August 30, 2019 at 10:22 AM
*To: *"Hohensee, Paul"
*Cc: *OpenJDK Serviceability ,
"hotspot-gc-...@openjdk.java.net"
*Subject: *Re: RFR (M): 8207266:
ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
OK. That's better. Some review comments:
The javadoc of getCurrentThreadAllocatedBytes() can simply say:
"Returns an approximation of the total amount of memory, in bytes,
allocated in heap memory for the current thread.
This is a convenient method for local management use and is equivalent
to calling getThreadAllocatedBytes(Thread.currentThread().getId()).
src/hotspot/share/include/jmm.h
GetOneThreadsAllocatedMemory: s/OneThreads/OneThread/
sun/management/ThreadImpl.java
43 private static final String
THREAD_ALLOCATED_MEMORY_NOT_SUPPORTED =
44 "Thread allocated memory measurement is not supported.";
if (!isThreadAllocatedMemorySupported()) {
throw new
UnsupportedOperationException(THREAD_ALLOCATED_MEMORY_NOT_SUPPORTED);
}
Perhaps the above can be refactored as
throwIfAllocatedMemoryUnsupported() method.
391 if (ids.length == 1) {
392 sizes[0] = -1;
:
398 if (ids.length == 1) {
399 long id = ids[0];
400 sizes[0] = getThreadAllocatedMemory0(
401 Thread.currentThread().getId() == id ? 0 : id);
402 } else {
It seems cleaner to handle the 1-element array case at the beginning
of this method:
if (ids.length == 1) {
long size = getThreadAllocatedBytes(ids[0]);
return new long[] { size };
}
I didn't review the hotspot implementation and the test.
Mandy
On 8/29/19 10:01 AM, Hohensee, Paul wrote:
My bad, Mandy. The webrev puts getCurrentThreadAllocatedBytes in
com.sun.management.ThreadMXBean along with the current two
getThreadAllocatedBytes methods for the reasons you list. I’ve
updated the CSR to specify com.sun.management and added a
rationale. AllocatedBytes is currently enabled by Hotspot by
default because the overhead of recording TLAB occupancy is
negligible.
There’s no new GC code, nor will there be, so imo we don’t have to
involve the GC folks. I.e., the new JMM method
GetOneThreadsAllocatedBytes uses the existing
cooked_allocated_bytes JavaThread method, and
getCurrentThreadAllocatedBytes is the same as
getThreadAllocatedBytes: it just bypasses the thread lookup code.
I hadn’t tracked down what happens when getCurrentThreadUserTime
and getCurrentThreadCpuTime are called before, but if I’m not
mistaken, it the code in jcmd() in attachListener.cpp will call
GetThreadCpuTimeWithKind in management.cpp, and it will ultimately
use Thread::current() as the subject of the call, see
os::current_thread_cpu_time in os_linux.cpp. That means that the
CurrentThread methods should work remotely the same way they do
locally. GetOneThreadsAllocatedBytes in management.cpp uses THREAD
as its subject when called on behalf of
getCurrentThreadAllocatedBytes, so it will also uses the current
remote Java thread. Even if these methods only worked locally,
there are many setups where apps are self-monitoring that could
use the performance improvement.
Thanks,
Paul
*From: *Mandy Chung
<mailto:mandy.ch...@oracle.com>
*Date: *Wed
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
Thanks for your review, Mandy. Revised webrev at
http://cr.openjdk.java.net/~phh/8207266/webrev.02/.
I updated the CSR with your suggested javadoc for
getCurrentThreadAllocatedBytes. It now matches that for
getCurrentThreadUserTime and getCurrentThreadCputime. I also fixed the
“convenient” -> “convenience” typos in j.l.m.ThreadMXBean.java.
I meant GetOneThreads to be the possessive, but don’t feel strongly either way
so I’m fine with GetOneThread.
I updated ThreadImpl.java as you suggested, though in
getThreadAllocatedBytes(long[] ids) I had to add a
redundant-in-the-not-length-1-case check for a null ids reference.
Would someone take a look at the Hotspot side and the test please?
Paul
From: Mandy Chung
Date: Friday, August 30, 2019 at 10:22 AM
To: "Hohensee, Paul"
Cc: OpenJDK Serviceability ,
"hotspot-gc-...@openjdk.java.net"
Subject: Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be
quicker for self thread
OK. That's better. Some review comments:
The javadoc of getCurrentThreadAllocatedBytes() can simply say:
"Returns an approximation of the total amount of memory, in bytes,
allocated in heap memory for the current thread.
This is a convenient method for local management use and is equivalent
to calling getThreadAllocatedBytes(Thread.currentThread().getId()).
src/hotspot/share/include/jmm.h
GetOneThreadsAllocatedMemory: s/OneThreads/OneThread/
sun/management/ThreadImpl.java
43 private static final String THREAD_ALLOCATED_MEMORY_NOT_SUPPORTED =
44 "Thread allocated memory measurement is not supported.";
if (!isThreadAllocatedMemorySupported()) {
throw new
UnsupportedOperationException(THREAD_ALLOCATED_MEMORY_NOT_SUPPORTED);
}
Perhaps the above can be refactored as throwIfAllocatedMemoryUnsupported()
method.
391 if (ids.length == 1) {
392 sizes[0] = -1;
:
398 if (ids.length == 1) {
399 long id = ids[0];
400 sizes[0] = getThreadAllocatedMemory0(
401 Thread.currentThread().getId() == id ? 0 : id);
402 } else {
It seems cleaner to handle the 1-element array case at the beginning
of this method:
if (ids.length == 1) {
long size = getThreadAllocatedBytes(ids[0]);
return new long[] { size };
}
I didn't review the hotspot implementation and the test.
Mandy
On 8/29/19 10:01 AM, Hohensee, Paul wrote:
My bad, Mandy. The webrev puts getCurrentThreadAllocatedBytes in
com.sun.management.ThreadMXBean along with the current two
getThreadAllocatedBytes methods for the reasons you list. I’ve updated the CSR
to specify com.sun.management and added a rationale. AllocatedBytes is
currently enabled by Hotspot by default because the overhead of recording TLAB
occupancy is negligible.
There’s no new GC code, nor will there be, so imo we don’t have to involve the
GC folks. I.e., the new JMM method GetOneThreadsAllocatedBytes uses the
existing cooked_allocated_bytes JavaThread method, and
getCurrentThreadAllocatedBytes is the same as getThreadAllocatedBytes: it just
bypasses the thread lookup code.
I hadn’t tracked down what happens when getCurrentThreadUserTime and
getCurrentThreadCpuTime are called before, but if I’m not mistaken, it the code
in jcmd() in attachListener.cpp will call GetThreadCpuTimeWithKind in
management.cpp, and it will ultimately use Thread::current() as the subject of
the call, see os::current_thread_cpu_time in os_linux.cpp. That means that the
CurrentThread methods should work remotely the same way they do locally.
GetOneThreadsAllocatedBytes in management.cpp uses THREAD as its subject when
called on behalf of getCurrentThreadAllocatedBytes, so it will also uses the
current remote Java thread. Even if these methods only worked locally, there
are many setups where apps are self-monitoring that could use the performance
improvement.
Thanks,
Paul
From: Mandy Chung <mailto:mandy.ch...@oracle.com>
Date: Wednesday, August 28, 2019 at 3:59 PM
To: "Hohensee, Paul" <mailto:hohen...@amazon.com>
Cc: OpenJDK Serviceability
<mailto:serviceability-dev@openjdk.java.net>,
"hotspot-gc-...@openjdk.java.net"<mailto: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,
The CSR proposes this method in java.lang.management.ThreadMXBean as a Java SE
feature.
Has this been discussed with the GC team to commit measuring current thread's
allocated bytes as Java SE feature? Can this be supported by all JVM
implementation? What is the overhead if this is enabled by default? Does it
need to be disabled? This metric is from TLAB that might be okay. This needs
advice/discussion with GC experts.
I see that CSR mentions it can be disabled and link to
isThreadA
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
OK. That's better. Some review comments:
The javadoc of getCurrentThreadAllocatedBytes() can simply say:
"Returns an approximation of the total amount of memory, in bytes,
allocated in heap memory for the current thread.
This is a convenient method for local management use and is equivalent
to calling getThreadAllocatedBytes(Thread.currentThread().getId()).
src/hotspot/share/include/jmm.h
GetOneThreadsAllocatedMemory: s/OneThreads/OneThread/
sun/management/ThreadImpl.java
43 private static final String
THREAD_ALLOCATED_MEMORY_NOT_SUPPORTED =
44 "Thread allocated memory measurement is not supported.";
if (!isThreadAllocatedMemorySupported()) {
throw new
UnsupportedOperationException(THREAD_ALLOCATED_MEMORY_NOT_SUPPORTED);
}
Perhaps the above can be refactored as
throwIfAllocatedMemoryUnsupported() method.
391 if (ids.length == 1) {
392 sizes[0] = -1;
:
398 if (ids.length == 1) {
399 long id = ids[0];
400 sizes[0] = getThreadAllocatedMemory0(
401 Thread.currentThread().getId() == id ? 0 : id);
402 } else {
It seems cleaner to handle the 1-element array case at the beginning
of this method:
if (ids.length == 1) {
long size = getThreadAllocatedBytes(ids[0]);
return new long[] { size };
}
I didn't review the hotspot implementation and the test.
Mandy
On 8/29/19 10:01 AM, Hohensee, Paul wrote:
My bad, Mandy. The webrev puts getCurrentThreadAllocatedBytes in
com.sun.management.ThreadMXBean along with the current two
getThreadAllocatedBytes methods for the reasons you list. I’ve updated
the CSR to specify com.sun.management and added a rationale.
AllocatedBytes is currently enabled by Hotspot by default because the
overhead of recording TLAB occupancy is negligible.
There’s no new GC code, nor will there be, so imo we don’t have to
involve the GC folks. I.e., the new JMM method
GetOneThreadsAllocatedBytes uses the existing cooked_allocated_bytes
JavaThread method, and getCurrentThreadAllocatedBytes is the same as
getThreadAllocatedBytes: it just bypasses the thread lookup code.
I hadn’t tracked down what happens when getCurrentThreadUserTime and
getCurrentThreadCpuTime are called before, but if I’m not mistaken, it
the code in jcmd() in attachListener.cpp will call
GetThreadCpuTimeWithKind in management.cpp, and it will ultimately use
Thread::current() as the subject of the call, see
os::current_thread_cpu_time in os_linux.cpp. That means that the
CurrentThread methods should work remotely the same way they do
locally. GetOneThreadsAllocatedBytes in management.cpp uses THREAD as
its subject when called on behalf of getCurrentThreadAllocatedBytes,
so it will also uses the current remote Java thread. Even if these
methods only worked locally, there are many setups where apps are
self-monitoring that could use the performance improvement.
Thanks,
Paul
*From: *Mandy Chung
*Date: *Wednesday, August 28, 2019 at 3:59 PM
*To: *"Hohensee, Paul"
*Cc: *OpenJDK Serviceability ,
"hotspot-gc-...@openjdk.java.net"
*Subject: *Re: RFR (M): 8207266:
ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
Hi Paul,
The CSR proposes this method in java.lang.management.ThreadMXBean as a
Java SE feature.
Has this been discussed with the GC team to commit measuring current
thread's allocated bytes as Java SE feature? Can this be supported
by all JVM implementation? What is the overhead if this is enabled
by default? Does it need to be disabled? This metric is from TLAB
that might be okay. This needs advice/discussion with GC experts.
I see that CSR mentions it can be disabled and link to
isThreadAllocatedMemoryEnabled() and setThreadAllocatedMemoryEnabled()
methods but these methods are defined in com.sun.management.ThreadMXBean.
As Alan points out, current thread makes sense only in local VM
management. When this is monitored from a JMX client (e.g. jconsole
to connect to a running JVM, "currentThreadAllowcatedBytes" attribute
is the current thread in jconsole process which invoking
Thread::currentThread?
Mandy
On 8/28/19 12:22 PM, Hohensee, Paul wrote:
Please review a performance improvement for
ThreadMXBean.getThreadAllocatedBytes and the addition of
getCurrentThreadAllocatedBytes.
JBS issue:https://bugs.openjdk.java.net/browse/JDK-8207266
Webrev:http://cr.openjdk.java.net/~phh/8207266/webrev.00/
CSR:https://bugs.openjdk.java.net/browse/JDK-8230311
Previous email threads:
https://mail.openjdk.java.net/pipermail/serviceability-dev/2018-July/024441.html
https://mail.openjdk.java.net/pipermail/serviceability-dev/2018-August/024763.html
The CSR is for adding ThreadMXBean.getCurrentThreadAllocatedBytes.
I’d be great for someone to review it.
I took Mandy’s advice and put the
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
Yes. See previous email. Thanks, On 8/29/19, 12:19 AM, "Alan Bateman" wrote: On 28/08/2019 23:58, Mandy Chung wrote: > Hi Paul, > > The CSR proposes this method in java.lang.management.ThreadMXBean as a > Java SE feature. > > Has this been discussed with the GC team to commit measuring current > thread's allocated bytes as Java SE feature? Can this be supported > by all JVM implementation? What is the overhead if this is enabled > by default? Does it need to be disabled? This metric is from TLAB > that might be okay. This needs advice/discussion with GC experts. The webrev adds it to jdk.management/com.sun.management.ThreadMXBean so I suspect it is a typo in the CSR and the proposal is for it to be JDK-specific. -Alan.
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
On 28/08/2019 23:58, Mandy Chung wrote: Hi Paul, The CSR proposes this method in java.lang.management.ThreadMXBean as a Java SE feature. Has this been discussed with the GC team to commit measuring current thread's allocated bytes as Java SE feature? Can this be supported by all JVM implementation? What is the overhead if this is enabled by default? Does it need to be disabled? This metric is from TLAB that might be okay. This needs advice/discussion with GC experts. The webrev adds it to jdk.management/com.sun.management.ThreadMXBean so I suspect it is a typo in the CSR and the proposal is for it to be JDK-specific. -Alan.
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
Hi Paul, The CSR proposes this method in java.lang.management.ThreadMXBean as a Java SE feature. Has this been discussed with the GC team to commit measuring current thread's allocated bytes as Java SE feature? Can this be supported by all JVM implementation? What is the overhead if this is enabled by default? Does it need to be disabled? This metric is from TLAB that might be okay. This needs advice/discussion with GC experts. I see that CSR mentions it can be disabled and link to isThreadAllocatedMemoryEnabled() and setThreadAllocatedMemoryEnabled() methods but these methods are defined in com.sun.management.ThreadMXBean. As Alan points out, current thread makes sense only in local VM management. When this is monitored from a JMX client (e.g. jconsole to connect to a running JVM, "currentThreadAllowcatedBytes" attribute is the current thread in jconsole process which invoking Thread::currentThread? Mandy On 8/28/19 12:22 PM, Hohensee, Paul wrote: Please review a performance improvement for ThreadMXBean.getThreadAllocatedBytes and the addition of getCurrentThreadAllocatedBytes. JBS issue:https://bugs.openjdk.java.net/browse/JDK-8207266 Webrev:http://cr.openjdk.java.net/~phh/8207266/webrev.00/ CSR:https://bugs.openjdk.java.net/browse/JDK-8230311 Previous email threads: https://mail.openjdk.java.net/pipermail/serviceability-dev/2018-July/024441.html https://mail.openjdk.java.net/pipermail/serviceability-dev/2018-August/024763.html The CSR is for adding ThreadMXBean.getCurrentThreadAllocatedBytes. I’d be great for someone to review it. I took Mandy’s advice and put the fast paths in the library code. I added a new JMM method GetOneThreadsAllocatedBytes that works the same as GetThreadCpuTime: it uses a thread_id value of zero to distinguish the current thread. On my Mac laptop, the result runs 47x faster for the current thread than the old implementation. The 3 tests intest/jdk/com/sun/management/ThreadMXBean all pass. I added code to ThreadAllocatedMemory.java to test getCurrentThreadAllocatedBytes as well as variations on getThreadAllocatedBytes(id). A submit repo job is in progress. Thanks, Paul
Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
On 28/08/2019 20:22, Hohensee, Paul wrote: Please review a performance improvement for ThreadMXBean.getThreadAllocatedBytes and the addition of getCurrentThreadAllocatedBytes. JBS issue:https://bugs.openjdk.java.net/browse/JDK-8207266 Webrev:http://cr.openjdk.java.net/~phh/8207266/webrev.00/ CSR:https://bugs.openjdk.java.net/browse/JDK-8230311 Previous email threads: https://mail.openjdk.java.net/pipermail/serviceability-dev/2018-July/024441.html https://mail.openjdk.java.net/pipermail/serviceability-dev/2018-August/024763.html The CSR is for adding ThreadMXBean.getCurrentThreadAllocatedBytes. I’d be great for someone to review it. I suspect the new method needs to be specified to be for the local management case only, and it might able to useful to specify it to be the equivalent of getThreadAllocatedBytes(Thread.currentThread().getId()). -Alan
