RFR(S) 8189688: NMT: Report per-class load metadata information

[Zhengyu Gu]

Hi Thomas,

Thanks for the review. Please see comments inline.

On 10/23/2017 09:01 AM, Thomas Stüfe wrote:
> Hi Zhengyu,
> 
> this is a nice addition!
> 
> ------
> 
> General remarks:
> 
> We already have a multitude of printing functions in metaspace.cpp, in 
> MetaspaceAux I find:
> 
> void MetaspaceAux::print_on(outputStream* out)
> 
> and
> 
> void MetaspaceAux::print_on(outputStream* out, Metaspace::MetadataType 
> mdtype)
> void MetaspaceAux::dump(outputStream* out)
> 
> There even exists a cleanup issue: 
> https://bugs.openjdk.java.net/browse/JDK-8185034.
> 
> I am not happy that we add yet another printing function to this bunch, 
> which basically does the same. Is there any way to reuse the existing 
> functions?
> 
> If not, could we at least for now rename print_metadata() to something 
> like print_metadata_for_nmt(), to tell it apart from print_on etc? And 
> mark them for later cleanup and consolidation?

Yes, I noticed the existing print_on functions and thought about using 
them. However, none of them matches NMT convention, e.g. NMT uses "=" 
between label and value.

I will rename print_metadata to print_metadata_for_nmt.


> 
> ----------
> 
> About the numbers:
> 
> I am not sure who this output is for. Customers analyzing metaspace 
> shortage or hotspot developers debugging the metaspace allocation? 
> Depending on the answer I would change the output somewhat.
> 
> If this it is for customers, the numbers are too much and the names are 
> misleading. Especially the per-classloader numbers:
> 
> "Metadata   capacity=%10.2f%s used=%10.2f%s free=%10.2f%s waste=%10.2f%s"
> 
> If we only want to answer the question "how much metaspace does a 
> classloader use up?", either "capacity" or "used" should be sufficient. 
> It does not really matter much which, really, and for most cases the 
> numbers should be very similar.

> In this case I would omit "free" and "waste", because those are 
> implementation details of the metaspace chunk allocation and - provided 
> the implementation is correct - provide no useful information. "free" 
> just means "space left in current chunk" which is a implementation 
> detail. Once the chunk is used up, a new one is aquired. "waste" is only 
> one tiny part of how memory can be wasted in the current metaspace 
> implementation. It does not include waste in retired VirtualspaceNodes 
> or in free chunks idling in free lists.
>
As far as I know, NMT is largely used by JVM developers.

"free" and "waste" information, actually, is very important for us to 
understand some of issues with current implementation.
(e.g. https://bugs.openjdk.java.net/browse/JDK-8187338)

This patch is derived from our investigation of memory cost for classes, 
these numbers play important roles.

> The latter would be an important addition, regardless if this is for 
> customers or for us. Chunks idling in freelists can make a huge impact, 
> and unfortunately this may happen in perfectly valid cases. See e.g. our 
> JEP "https://bugs.openjdk.java.net/browse/JDK-8166690". We have already 
> a printing routine for free chunks, in 
> ChunkManager::print_all_chunkmanagers(). Could you add this to your output?

Make sense! Could you recommend what data and format you would like to see?


> 
> -------------
> 
> Other than above notes, the changes seem straighforward, I did not see 
> anything wrong. Minor nits:
> 
> - PrintCLDMetaspaceInfoClosure::_out, _scale and _unit could all be 
> constant (with _out being a outputStream* const).
> - We also do not need to provide scale *and* unit as argument, one can 
> be deduced from the other. This would also prevent mismatches.We do need scale here, since nmt command line can set scale and we do 
have to honor that.

However, passing unit is ugly! I don't want to introduce inverse 
dependence (metaspace -> nmt), which is also ugly.

Probably should move scale mapping code from NMTUtil to a common util?


> 
> I did not look closely at locking issues, I assume 
> MetaspaceAux::print_metadata() is supposed to run only in Safepoints?

No. sum_xxx_xxx_in_use queries are guarded by locks.

Thanks,

-Zhengyu

> 
> 
> Thanks & Kind Regards, Thomas
> 
> 
> 
> 
> 
> 
> On Fri, Oct 20, 2017 at 4:00 PM, Zhengyu Gu <zgu at redhat.com 
> <mailto:zgu at redhat.com>> wrote:
> 
>     Up to now, there is little visibility into how metaspaces are used,
>     and by whom.
> 
>     This proposed enhancement gives NMT the ability to report metaspace
>     usages on per-classloader level, via a new NMT command "metadata"
> 
> 
>     For example:
> 
>     2496:
>     Metaspaces:
>        Metadata space: reserved=      8192KB committed=      5888KB
>        Class    space: reserved=   1048576KB committed=       640KB
> 
>     Per-classloader metadata:
> 
>     ClassLoader: for anonymous class
>        Metadata   capacity=      5.00KB used=      1.16KB free=     
>     3.84KB waste=      0.05KB
>        Class data capacity=      1.00KB used=      0.59KB free=     
>     0.41KB waste=      0.00KB
> 
>     ....
> 
>     ClassLoader: <bootloader>
>        Metadata   capacity=   5640.00KB used=   5607.42KB free=   
>       32.58KB waste=      0.46KB
>        Class data capacity=    520.00KB used=    500.94KB free=   
>       19.06KB waste=      0.02KB
> 
> 
>     Bug: https://bugs.openjdk.java.net/browse/JDK-8189688
>     <https://bugs.openjdk.java.net/browse/JDK-8189688>
>     Webrev: http://cr.openjdk.java.net/~zgu/8189688/webrev.00/index.html
>     <http://cr.openjdk.java.net/~zgu/8189688/webrev.00/index.html>
> 
>     Test:
> 
>        hotspot_tier1_runtime, fastdebug and release on x64 Linux.
> 
>     Thanks,
> 
>     -Zhengyu
> 
> 
>