RFR(L): 8201572: Improve Metaspace Statistics

Wed May 2 11:58:13 UTC 2018


On 5/2/18 1:36 AM, Thomas Stüfe wrote:
> Hi Coleen,
>
> On Tue, May 1, 2018 at 3:54 PM,  <coleen.phillimore at oracle.com> wrote:
>> Hi, This isn't a full review but I like that you've added a metaspace
>> namespace and directory under memory for it.
>> This will make a good foundation for doing this change:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8176808
>>
> Yes, that is what I thought.
>
> When ripping up metaspace.cpp, should we be concerned that we loose
> history for the ripped out code?

The history will be there if one looks for it, or in older repositories.
thanks,
Coleen
>
> ..Thomas
>
>> Thanks,
>> Coleen
>>
>>
>> On 4/26/18 2:03 AM, Thomas Stüfe wrote:
>>> Hi all,
>>>
>>> I still need an additional reviewer.
>>>
>>> new Webrev:
>>> http://cr.openjdk.java.net/~stuefe/webrevs/8201572-improve-metaspace-reporting/webrev.02/webrev/index.html
>>> incremental to v1:
>>>
>>> http://cr.openjdk.java.net/~stuefe/webrevs/8201572-improve-metaspace-reporting/webrev-01-to-02/webrev/
>>>
>>> new: just build- and test fixes:
>>> - I added final cr() to a number of places where LogStream was used to
>>> pipe output from functions I changed. Otherwise ~LogStream will assert
>>> complaining about unfinished lines. I plan to change this, see
>>> https://bugs.openjdk.java.net/browse/JDK-8202303, however for now,
>>> lets go with the additional cr()
>>> - build fixes for MacOS and Solaris.
>>>
>>> jdk-submit test suite ran clean through. We are currently running the
>>> patch through our test suites, so far all looks good.
>>>
>>> Thanks, Thomas
>>>
>>>
>>>
>>>
>>> On Tue, Apr 24, 2018 at 6:28 PM, Thomas Stüfe <thomas.stuefe at gmail.com>
>>> wrote:
>>>> Hi all,
>>>>
>>>> New webrev:
>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8201572-improve-metaspace-reporting/webrev-01/webrev/
>>>> Incremental:
>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8201572-improve-metaspace-reporting/webrev-00-to-01/webrev/
>>>>
>>>> This one now cleanly applies to jdk-jdk and does not need other patches.
>>>>
>>>> Changes:
>>>> - removed MetaWord occurrences in ostream.cpp, replaces with BytesPerWord
>>>> - added per-spacetype printing to PrintNMTStatistics output to make it
>>>> more closely resemble the output before.  Example output:
>>>>
>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8201572-improve-metaspace-reporting/examples/PrintNMTStatistics.txt
>>>> - Fixed Windows build (missed precompiled.hpp)
>>>>
>>>> Thanks, Thomas
>>>>
>>>>
>>>> On Mon, Apr 23, 2018 at 6:32 PM, Thomas Stüfe <thomas.stuefe at gmail.com>
>>>> wrote:
>>>>> Hi all,
>>>>>
>>>>> may I please have opinions and reviews for the following improvement.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8201572
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8201572-improve-metaspace-reporting/webrev.00/webrev/
>>>>>
>>>>> (Note: it needs patch for 8202074 [3] to apply cleanly)
>>>>>
>>>>> This is a combined feature-add and cleanup for the metaspace
>>>>> reporting. It should have no effect on metaspace allocation itself.
>>>>>
>>>>> This change does a number of things:
>>>>>
>>>>> - It improves the "VM.metaspace" jcmd, making it hopefully much more
>>>>> versatile.
>>>>>
>>>>> - It improves the way statistics are collected: The number of times we
>>>>> walk the CLDG or the internal metaspace structures is reduced. It also
>>>>> adds another global counter to MetaspaceUtils.
>>>>>
>>>>> - Though this was not primarily intended as a cleanup change, a number
>>>>> of cleanups are part of this change. A number of functions were
>>>>> removed. Where new code was added, I attempted to define and follow a
>>>>> new scheme using namespaces and separate files. See below for details.
>>>>>
>>>>> ---
>>>>>
>>>>> Details:
>>>>>
>>>>> -> Improvements of jcmd:
>>>>>
>>>>> The jcmd "VM.metaspace" command now features a number of sub command
>>>>> which control the details printed:
>>>>>
>>>>> jcmd VM.metaspace [basic] [show-loaders] [by-chunktype] [by-spacetype]
>>>>> [vslist] [vsmap] [scale]
>>>>>
>>>>> All of them except for "basic" can be combined with each other.
>>>>>
>>>>> "basic"
>>>>>     shows a basic report; it needs no CLDG walk and no locks, so it
>>>>> should be safe under most conditions.
>>>>> "show-loaders"
>>>>>     this shows metaspace usage by loaders (basically what VM.metaspace
>>>>> did before - this was originally added by Zhengyu Gu)
>>>>> "by-chunktype"
>>>>>     Numbers are broken down by chunk type (small, medium...) in addition
>>>>> to total numbers
>>>>> "by-spacetype"
>>>>>     Numbers are broken down by space type (bootloader, anonymous,
>>>>> reflection...) in addition to total numbers
>>>>> "vslist"
>>>>>     The virtual space list is printed in detail
>>>>> "vsmap"
>>>>>     Prints the virtual space map (ascii map of chunk placements)
>>>>>
>>>>> When no arguments are given, a summary is displayed.
>>>>>
>>>>> In addition to the above mentioned features we now print
>>>>> - a "Waste" section detailing wastage.
>>>>> - in debug mode, a section with some internal counters. This was just
>>>>> curiosity on my part, I wanted to see how "hot" some of the metaspace
>>>>> allocation paths actually get.
>>>>> - We now count overhead too, i.e. space needed for chunk headers (was
>>>>> counted as "used" before)
>>>>> - We now also show deallocated space (block freelist)
>>>>>
>>>>>
>>>>> Some output examples:
>>>>>
>>>>> <No arguments>
>>>>>
>>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8201572-improve-metaspace-reporting/examples/VM.metaspace.txt
>>>>>
>>>>> "basic"
>>>>>
>>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8201572-improve-metaspace-reporting/examples/VM.metaspace_basic.txt
>>>>>
>>>>> "by-chunktype"
>>>>>
>>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8201572-improve-metaspace-reporting/examples/VM.metaspace_by-chunktype.txt
>>>>>
>>>>> "by-spacetype"
>>>>>
>>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8201572-improve-metaspace-reporting/examples/VM.metaspace_by_spacetype.txt
>>>>>
>>>>> "show-loaders"
>>>>>
>>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8201572-improve-metaspace-reporting/examples/VM.metaspace_showloaders.txt
>>>>>
>>>>> "show-loaders" and "by-chunktype combined"
>>>>>
>>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8201572-improve-metaspace-reporting/examples/VM.metaspace_showloaders_by-chunktype.txt
>>>>>
>>>>> "vslist"
>>>>>
>>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8201572-improve-metaspace-reporting/examples/VM.metaspace_vslist.txt
>>>>>
>>>>> "vsmap"
>>>>>
>>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8201572-improve-metaspace-reporting/examples/VM.metaspace_vsmap.txt
>>>>>
>>>>> ---
>>>>>
>>>>> -> The way we collect statistics was streamlined. I tried to get rid
>>>>> of all functions which were walking structures to just sum up a single
>>>>> value; not only was that unnecessary code duplication, it was also
>>>>> inefficient.
>>>>>
>>>>> This means most of the SpaceManager::sum_xxxx() functions are gone, as
>>>>> well as all the different MetaspaceUtils::xxx_xxx_slow() functions.
>>>>>
>>>>> They were replaced by functions named "collect_statistics" or
>>>>> "add_statistics", which accumulate all needed data in one go into a
>>>>> statistics structure. This is more efficient since we usually need
>>>>> more than one value and so it makes sense to collect them in one go.
>>>>>
>>>>> The newly added statistics objects are:
>>>>>
>>>>> - UsedChunksStatistics
>>>>> - SpaceManagerStatistics
>>>>> - ClassLoaderMetaspaceStatistics
>>>>>
>>>>> They live in a new file (memory/metaspace/metaspaceStatistics).
>>>>>
>>>>> I also ripped out the preexisting ChunkManagerStatistics, modified it
>>>>> to follow the same convention, and moved it into this new file.
>>>>>
>>>>> -> We now have a new central reporting function,
>>>>> MetaspaceUtils::print_report() - this was once Zhengyu's
>>>>> print_for_nmt() function, but I expanded it and renamed it to be the
>>>>> central entry point. Reporting options are handed into it via flags.
>>>>>
>>>>> In addition to that, we have a new
>>>>> MetaspaceUtils::print_basic_report(), which prints a short summary
>>>>> under the promise of not walking or locking anything. This is used at
>>>>> error reporting time to print a short summary to the hs-err file. It
>>>>> is also used for VM.info().
>>>>>
>>>>> -> All new functions now accept the "scale" parameter known from NMT.
>>>>> I expanded it a bit to have a "dynamic" setting. In that setting, the
>>>>> printer chooses the best unit for a given value. I find it works quite
>>>>> well, so I made it the default. I also changed printing to correctly
>>>>> print very-small-but-not-0 numbers or percentages, where before they
>>>>> were rounded to 0. See linked output examples.
>>>>>
>>>>> -> Other things to note:
>>>>>
>>>>> - MetaspaceUtils::dump was used to print some details at Metaspace
>>>>> OOM. This now calls MetaspaceUtils::print_basic_report() directly, and
>>>>> I removed the dump() function.
>>>>>
>>>>> - I moved printing the summary out of the PrintCLDMetaspaceInfoClosure
>>>>> and into the print_report function. PrintCLDMetaspaceInfoClosure still
>>>>> prints the individual loader metaspace usage numbers.
>>>>>
>>>>> - I streamlined naming of values somewhat. Where we refer to in-use
>>>>> chunks, I now use consistently "capacity", "free", "waste", "used" and
>>>>> the new "overhead", and the assumption is that capacity is the sum of
>>>>> all the other values.
>>>>>
>>>>> - About the newly added namespaces: Since the patch is already quite
>>>>> big, I did not want to load it with more cleanup. However, neither did
>>>>> I want to just dump the new classes and functions into metaspace.cpp.
>>>>> So as a compromise I tried to follow a new scheme with newly added
>>>>> code. If you agree on this organization, this may be the way to split
>>>>> metaspace.cpp in some follow up change.
>>>>>
>>>>> The idea was to move all metaspace-internal stuff into
>>>>> memory/metaspace and hide it from the outside. Also, I did put
>>>>> metaspace coding into an own namespace ("metaspace") and
>>>>> metaspace-internal coding into "metaspace::internal". This is just a
>>>>> proposal, we can bike shed. Future changes could move more classes out
>>>>> of metaspace.cpp into metaspace/ and the metaspace::internal namespace
>>>>> to make metaspace.cpp malleable again.
>>>>>
>>>>> Thank you,
>>>>>
>>>>> Kind Regards, Thomas
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --------------------
>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8201572
>>>>> [2]
>>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8201572-improve-metaspace-reporting/webrev.00/webrev/
>>>>> [3]
>>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8202074-metaspace-retire-after-humongous-chunks-are-allocated/webrev.00/webrev/
>>