RFR(XXS) 8190357: NMT: Include metadata information in NMT final report when PrintNMTStatistics is on

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR(XXS) 8190357: NMT: Include metadata information in NMT final report when PrintNMTStatistics is on

Zhengyu Gu-2
This patch adds metadata reporting in NMT final report.

What complicates the matter, is that, reporting per-class loader
metadata requires a safepoint, so that it can safely walk class loaders.
So, we only report it when JVM is about to exit in good state.


Bug: https://bugs.openjdk.java.net/browse/JDK-8190357
Webrev: http://cr.openjdk.java.net/~zgu/8190357/webrev.00/


Thanks,

-Zhengyu
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XXS) 8190357: NMT: Include metadata information in NMT final report when PrintNMTStatistics is on

Andrew Dinn
Hi Zhengyu,

On 09/11/17 19:33, Zhengyu Gu wrote:
> This patch adds metadata reporting in NMT final report.
>
> What complicates the matter, is that, reporting per-class loader
> metadata requires a safepoint, so that it can safely walk class loaders.
> So, we only report it when JVM is about to exit in good state.
>
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8190357
> Webrev: http://cr.openjdk.java.net/~zgu/8190357/webrev.00/
I understand the need here to avoid reporting if we are under a fatal
error. However, that assert is not going to work in product code. So,
does that not imply that execution of the vmop needs to be conditional
on VMError::fatal_error_in_progress() returning false?

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XXS) 8190357: NMT: Include metadata information in NMT final report when PrintNMTStatistics is on

Zhengyu Gu-2
Hi Andrew,

On 11/10/2017 04:22 AM, Andrew Dinn wrote:

> Hi Zhengyu,
>
> On 09/11/17 19:33, Zhengyu Gu wrote:
>> This patch adds metadata reporting in NMT final report.
>>
>> What complicates the matter, is that, reporting per-class loader
>> metadata requires a safepoint, so that it can safely walk class loaders.
>> So, we only report it when JVM is about to exit in good state.
>>
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8190357
>> Webrev: http://cr.openjdk.java.net/~zgu/8190357/webrev.00/
> I understand the need here to avoid reporting if we are under a fatal
> error. However, that assert is not going to work in product code. So,
> does that not imply that execution of the vmop needs to be conditional
> on VMError::fatal_error_in_progress() returning false?

MemTracker::report() is a private method, only called by
MemTracker::final_report() and MemTracker::error_report(). This
assertion ensures that metadata report should never be included in
MemTracker::error_report() if it decides to extend beyond summary report.

Of course, this is under assumption that final_report() is called during
JVM normal shutdown (good state) and error_report() is by error handler
(bad state).

Besides, VMError::fatal_error_in_progress() is inherently racy.

Thanks,

-Zhengyu


>
> regards,
>
>
> Andrew Dinn
> -----------
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XXS) 8190357: NMT: Include metadata information in NMT final report when PrintNMTStatistics is on

Thomas Stüfe-2
In reply to this post by Zhengyu Gu-2
Hi Zhengyu,

 Cannot test it before monday, but change looks good. Thank you for doing
this.

..Thomas

On Thu 9. Nov 2017 at 20:33, Zhengyu Gu <[hidden email]> wrote:

> This patch adds metadata reporting in NMT final report.
>
> What complicates the matter, is that, reporting per-class loader
> metadata requires a safepoint, so that it can safely walk class loaders.
> So, we only report it when JVM is about to exit in good state.
>
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8190357
> Webrev: http://cr.openjdk.java.net/~zgu/8190357/webrev.00/
>
>
> Thanks,
>
> -Zhengyu
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XXS) 8190357: NMT: Include metadata information in NMT final report when PrintNMTStatistics is on

Zhengyu Gu-2


On 11/10/2017 08:20 AM, Thomas Stüfe wrote:
> Hi Zhengyu,
>
>   Cannot test it before monday, but change looks good. Thank you for
> doing this.

No problem.

Thanks,

-Zhengyu

>
> ..Thomas
>
> On Thu 9. Nov 2017 at 20:33, Zhengyu Gu <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     This patch adds metadata reporting in NMT final report.
>
>     What complicates the matter, is that, reporting per-class loader
>     metadata requires a safepoint, so that it can safely walk class loaders.
>     So, we only report it when JVM is about to exit in good state.
>
>
>     Bug: https://bugs.openjdk.java.net/browse/JDK-8190357
>     Webrev: http://cr.openjdk.java.net/~zgu/8190357/webrev.00/
>
>
>     Thanks,
>
>     -Zhengyu
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XXS) 8190357: NMT: Include metadata information in NMT final report when PrintNMTStatistics is on

Andrew Dinn
In reply to this post by Zhengyu Gu-2
On 10/11/17 12:47, Zhengyu Gu wrote:
> On 11/10/2017 04:22 AM, Andrew Dinn wrote:
<snip>

>> I understand the need here to avoid reporting if we are under a fatal
>> error. However, that assert is not going to work in product code. So,
>> does that not imply that execution of the vmop needs to be conditional
>> on VMError::fatal_error_in_progress() returning false?
>
> MemTracker::report() is a private method, only called by
> MemTracker::final_report() and MemTracker::error_report(). This
> assertion ensures that metadata report should never be included in
> MemTracker::error_report() if it decides to extend beyond summary report.
>
> Of course, this is under assumption that final_report() is called during
> JVM normal shutdown (good state) and error_report() is by error handler
> (bad state).

Ok, thanks for the explanation. The change looks fine.

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XXS) 8190357: NMT: Include metadata information in NMT final report when PrintNMTStatistics is on

Zhengyu Gu-2
Thanks for the review, Andrew.

-Zhengyu

On 11/10/2017 09:08 AM, Andrew Dinn wrote:

> On 10/11/17 12:47, Zhengyu Gu wrote:
>> On 11/10/2017 04:22 AM, Andrew Dinn wrote:
> <snip>
>>> I understand the need here to avoid reporting if we are under a fatal
>>> error. However, that assert is not going to work in product code. So,
>>> does that not imply that execution of the vmop needs to be conditional
>>> on VMError::fatal_error_in_progress() returning false?
>>
>> MemTracker::report() is a private method, only called by
>> MemTracker::final_report() and MemTracker::error_report(). This
>> assertion ensures that metadata report should never be included in
>> MemTracker::error_report() if it decides to extend beyond summary report.
>>
>> Of course, this is under assumption that final_report() is called during
>> JVM normal shutdown (good state) and error_report() is by error handler
>> (bad state).
>
> Ok, thanks for the explanation. The change looks fine.
>
> regards,
>
>
> Andrew Dinn
> -----------
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XXS) 8190357: NMT: Include metadata information in NMT final report when PrintNMTStatistics is on

Zhengyu Gu-2
In reply to this post by Thomas Stüfe-2
Hi Thomas,

Have you gotten chance to review this? I would like to get this in
before door closes for jdk10.

Thanks,

-Zhengyu



On 11/10/2017 08:20 AM, Thomas Stüfe wrote:

> Hi Zhengyu,
>
>   Cannot test it before monday, but change looks good. Thank you for
> doing this.
>
> ..Thomas
>
> On Thu 9. Nov 2017 at 20:33, Zhengyu Gu <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     This patch adds metadata reporting in NMT final report.
>
>     What complicates the matter, is that, reporting per-class loader
>     metadata requires a safepoint, so that it can safely walk class loaders.
>     So, we only report it when JVM is about to exit in good state.
>
>
>     Bug: https://bugs.openjdk.java.net/browse/JDK-8190357
>     Webrev: http://cr.openjdk.java.net/~zgu/8190357/webrev.00/
>
>
>     Thanks,
>
>     -Zhengyu
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XXS) 8190357: NMT: Include metadata information in NMT final report when PrintNMTStatistics is on

Zhengyu Gu-2
Thanks for the review, Thomas.

-Zhengyu


On 11/16/2017 07:27 PM, Thomas Stüfe wrote:

> Hi zhengyu,
>
> The change looks fine. No objection from my side.
>
> Thanks, Thomas
>
> On Fri 17. Nov 2017 at 00:31, Zhengyu Gu <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Hi Thomas,
>
>     Have you gotten chance to review this? I would like to get this in
>     before door closes for jdk10.
>
>     Thanks,
>
>     -Zhengyu
>
>
>
>     On 11/10/2017 08:20 AM, Thomas Stüfe wrote:
>     > Hi Zhengyu,
>     >
>     >   Cannot test it before monday, but change looks good. Thank you for
>     > doing this.
>     >
>     > ..Thomas
>     >
>     > On Thu 9. Nov 2017 at 20:33, Zhengyu Gu <[hidden email]
>     <mailto:[hidden email]>
>     > <mailto:[hidden email] <mailto:[hidden email]>>> wrote:
>     >
>     >     This patch adds metadata reporting in NMT final report.
>     >
>     >     What complicates the matter, is that, reporting per-class loader
>     >     metadata requires a safepoint, so that it can safely walk
>     class loaders.
>     >     So, we only report it when JVM is about to exit in good state.
>     >
>     >
>     >     Bug: https://bugs.openjdk.java.net/browse/JDK-8190357
>     >     Webrev: http://cr.openjdk.java.net/~zgu/8190357/webrev.00/
>     <http://cr.openjdk.java.net/%7Ezgu/8190357/webrev.00/>
>     >
>     >
>     >     Thanks,
>     >
>     >     -Zhengyu
>     >
>