RFR(S) 8189688: NMT: Report per-class load metadata information
Thomas Stüfe
thomas.stuefe at gmail.com
Tue Oct 24 16:08:18 UTC 2017
Hi Zhengyu,
- VM_PrintMetadata still has the _unit member, but I see it nowhere used.
- I would have split the summary between class and non-class area, that may
have been more useful. But this is a matter of taste, I leave it up to you.
All else looks fine to me now. I do not need another review.
Thanks for your work, this will come in handy!
..Thomas
On Tue, Oct 24, 2017 at 5:08 PM, Zhengyu Gu <zgu at redhat.com> wrote:
> Hi Thomas,
>
>
>> Not sure whether we misunderstood each other. I was thinking of something
>> in the line of:
>>
>> <<<<
>> ....
>> ClassLoader: jdk/internal/reflect/DelegatingClassLoader
>> Metadata:
>> capacity: 5.00KB used: 3.32KB free: 1.68KB waste: 0.00KB
>> Class data:
>> capacity: 1.00KB used: 0.54KB free: 0.46KB waste: 0.00KB
>>
>> ClassLoader: for anonymous class
>> Metadata:
>> capacity: 1.00KB used: 1.00KB free: 0.00KB waste: 0.00KB
>> Class data:
>> capacity: 1.00KB used: 0.64KB free: 0.36KB waste: 0.00KB
>> ....
>>
>> Summary:
>> XX class loaders encountered, total capacity: xx, total waste: xx.
>>
>> Peak usage by class loader: xxxx
>> >>>>
>>
> Added summary lines:
>
> Total class loaders= 56 capacity= 6378.00KB used= 6205.08KB
> free= 172.92KB waste= 1.44KB
> For anonymous classes= 54 capacity= 212.00KB used= 96.95KB
> free= 115.05KB waste= 0.94KB
>
>
>
>
>> These numbers would not be interpretation, just a convenience to the
>> reader to get a clear picture.
>>
>> It even may be useful to separate the output between basic and detailed
>> mode, and in basic mode omit all the single class loaders and just print
>> the summary line.
>>
>> Updated webrev:
>> http://cr.openjdk.java.net/~zgu/8189688/webrev.01/index.html
>> <http://cr.openjdk.java.net/~zgu/8189688/webrev.01/index.html>
>>
>>
>>
>> metaspace.hpp:
>>
>> I would have preferred to keep scale_unit file local static instead of
>> exposing it via MetaspaceAux, because it does not really have anything to
>> do with metaspace.
>>
> Fixed
>
>
>> Otherwise ok.
>>
>> ---
>>
>> metaspace.cpp
>>
>> - ChunkManager::print_statistics(): thanks for reusing this function!
>> -> Scale can only be ever 1, K, M, G, yes? So, could you add an
>> assert at the start of the function, and a comment at the prototype or
>> function head?
>> -> Also, now that ChunkManager::print_statistics() takes a scale,
>> could you please change the printout to use floats like you did in
>> PrintCLDMetaspaceInfoClosure::print_metaspace()?
>>
> Done.
>
>
> Chunkmanager (non-class):
> 0 specialized (128 bytes) chunks, total 0.00KB
> 0 small (512 bytes) chunks, total 0.00KB
> 0 medium (8192 bytes) chunks, total 0.00KB
> 0 humongous chunks, total 0.00KB
> total size: 0.00KB.
> Chunkmanager (class):
> 0 specialized (128 bytes) chunks, total 0.00KB
> 0 small (256 bytes) chunks, total 0.00KB
> 0 medium (4096 bytes) chunks, total 0.00KB
> 0 humongous chunks, total 0.00KB
> total size: 0.00KB.
>
>
>> - I am concerned about races in PrintCLDMetaspaceInfoClosure::do_cld(ClassLoaderData*
>> cld). Maybe my understanding is limited. We bail if cld->is_unloading. But
>> nothing prevents a ClassLoaderDataGraph from starting to unload a
>> ClassLoaderData and tearing down the Metaspace after we started printing it
>> in another thread, right? Also, I do not see any fences.
>>
>> So, I wonder whether PrintCLDMetaspaceInfoClosure should run in lock
>> protection. And then, I wonder if it would be good to separate data
>> gathering and printing. We print to a (at least in principle) unknown
>> output stream, which may be doing slow File IO or even Network IO. Doing
>> this under lock protection seems not a good idea (but I see other places
>> where this happens too).
>>
>> For an example, see ChunkManager::get_statistic() vs
>> ChunkManager::print_statistic(). Could you do the same, have a data
>> gathering step where you collect your <capacity/used/free/waste> quadrupel
>> in a variable sized list of structures in memory, and print it out in a
>> separate step? I realize though that the effort would be larger than for
>> what we did in ChunkManager::get_statistic(), where we only fill a
>> structure.
>>
>>
> Unlike other NMT queries, this query is executed at a safepoint by
> VMThread, so it should be okay.
>
> Updated webrev: http://cr.openjdk.java.net/~zgu/8189688/webrev.02/
>
> Thanks,
>
> -Zhengyu
>
>
> ------
>>
>> vm_operations.hpp
>>
>> - VM_PrintMetadata : do we still need _unit?
>>
>>
>> Thanks,
>>
>> -Zhengyu
>>
>>
>>
>> Thanks!
>>
>> Thomas
>>
>>
>>
>> On 10/23/2017 11:08 AM, Thomas Stüfe wrote:
>>
>>
>>
>> On Mon, Oct 23, 2017 at 5:03 PM, Zhengyu Gu <zgu at redhat.com
>> <mailto:zgu at redhat.com> <mailto:zgu at redhat.com
>> <mailto:zgu at redhat.com>>> wrote:
>>
>>
>>
>> Okay. So this is important for understanding cases
>> where we have
>> a large number of miniscule class loaders, each one
>> only having
>> a few numbers of chunks. In this case, would it be
>> useful to
>> have a running total of "free" and "waste", too? Or
>> even better,
>> also average and peak values of "free" and "waste" (to
>> tell
>> apart cases where you have one outlier vs cases where
>> just all
>> metaspaces waste a lot of memory).
>> Just out of curiosity, I looked at
>> http://cr.openjdk.java.net/~zgu/cld_metaspace/wildfly.txt
>> <http://cr.openjdk.java.net/~zgu/cld_metaspace/wildfly.txt>
>> <http://cr.openjdk.java.net/~z
>> gu/cld_metaspace/wildfly.txt
>> <http://cr.openjdk.java.net/~zgu/cld_metaspace/wildfly.txt>> and
>> it seems that "free" is the problem, not "waste"? So I
>> guess
>> what happens is that all the little classloaders
>> allocate just
>> enough memory to push them over "_small_chunk_limit=4",
>> so they
>> allocate the first medium chunk, from which then they
>> take a
>> small bite and leave the rest laying around?
>>
>> Yes. The free space of anonymous classes should be counted
>> as waste,
>> in my opinion. I am not sure if everyone agrees, so I took
>> the
>> summary line out of this patch.
>>
>> I would be more than happy to restore the summary line, if
>> you find
>> it is useful :-)
>>
>>
>> I agree with you. Typically class loaders stop loading at some
>> point, especially these tine ones, and often will not resume
>> loading. So, effectivly, the space is wasted. It still helps to
>> distinguish "free" in the current chunks from "free" in the
>> other chunks to tell this situation apart from a situation where
>> we have just a bug in metaspace chunk handling preventing us to
>> use up our chunks.
>>
>>
>> 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
>> <https://bugs.openjdk.java.net/browse/JDK-8166690>
>> <https://bugs.openjdk.java.net/browse/JDK-8166690
>> <https://bugs.openjdk.java.net/browse/JDK-8166690>>
>> <https://bugs.openjdk.java.net
>> /browse/JDK-8166690
>> <https://bugs.openjdk.java.net/browse/JDK-8166690>
>> <https://bugs.openjdk.java.net/browse/JDK-8166690
>> <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?
>>
>>
>>
>> Would not ChunkManager::print_all_chunkmanagers() be
>> sufficient?
>>
>> Okay, I might need to tweak output format.
>>
>>
>> Fine by me. Nothing depends on it yet.
>>
>> Thanks, Thomas
>>
>> Thanks,
>>
>> -Zhengyu
>>
>>
>>
>> -------------
>>
>> 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.
>>
>>
>> Yes, that would be regrettable.
>>
>> Probably should move scale mapping code from
>> NMTUtil to a
>> common util?
>>
>>
>>
>> That would be possible, these functions
>> (scale_from_name etc)
>> are simple enough to be moved to a generic file.
>> However, if you
>> pass scale, deducing the string that goes with it is
>> trivial and
>> I would have no qualms doing this by hand in
>> metaspace.cpp. But
>> it is a matter of taste.
>>
>>
>> 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, Thomas
>>
>>
>>
>> Thanks & Kind Regards, Thomas
>>
>>
>>
>>
>>
>>
>> On Fri, Oct 20, 2017 at 4:00 PM, Zhengyu Gu
>> <zgu at redhat.com <mailto:zgu at redhat.com>
>> <mailto:zgu at redhat.com <mailto:zgu at redhat.com>>
>> <mailto:zgu at redhat.com <mailto:zgu at redhat.com>
>> <mailto:zgu at redhat.com <mailto:zgu at redhat.com>>>
>> <mailto:zgu at redhat.com <mailto:zgu at redhat.com>
>> <mailto:zgu at redhat.com <mailto:zgu at redhat.com>>
>>
>> <mailto:zgu at redhat.com <mailto:zgu at redhat.com>
>> <mailto: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>
>> <https://bugs.openjdk.java.net/browse/JDK-8189688
>> <https://bugs.openjdk.java.net/browse/JDK-8189688>>
>> <https://bugs.openjdk.java.net
>> /browse/JDK-8189688
>> <https://bugs.openjdk.java.net/browse/JDK-8189688>
>> <https://bugs.openjdk.java.net/browse/JDK-8189688
>> <https://bugs.openjdk.java.net/browse/JDK-8189688>>>
>> <https://bugs.openjdk.java.net
>> /browse/JDK-8189688
>> <https://bugs.openjdk.java.net/browse/JDK-8189688>
>> <https://bugs.openjdk.java.net/browse/JDK-8189688
>> <https://bugs.openjdk.java.net/browse/JDK-8189688>>
>> <https://bugs.openjdk.java.net
>> /browse/JDK-8189688
>> <https://bugs.openjdk.java.net/browse/JDK-8189688>
>> <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>
>> <http://cr.openjdk.java.net/~z
>> gu/8189688/webrev.00/index.html
>> <http://cr.openjdk.java.net/~zgu/8189688/webrev.00/index.html>>
>> <http://cr.openjdk.java.net/~z
>> gu/8189688/webrev.00/index.html
>> <http://cr.openjdk.java.net/~zgu/8189688/webrev.00/index.html>
>> <http://cr.openjdk.java.net/~z
>> gu/8189688/webrev.00/index.html
>> <http://cr.openjdk.java.net/~zgu/8189688/webrev.00/index.html>>>
>> <
>> http://cr.openjdk.java.net/~zgu/8189688/webrev.00/index.html
>> <http://cr.openjdk.java.net/~zgu/8189688/webrev.00/index.html>
>> <http://cr.openjdk.java.net/~z
>> gu/8189688/webrev.00/index.html
>> <http://cr.openjdk.java.net/~zgu/8189688/webrev.00/index.html>>
>> <http://cr.openjdk.java.net/~z
>> gu/8189688/webrev.00/index.html
>> <http://cr.openjdk.java.net/~zgu/8189688/webrev.00/index.html>
>> <http://cr.openjdk.java.net/~z
>> gu/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
>>
>>
>>
>>
>>
>>
>>
More information about the hotspot-runtime-dev
mailing list