RFR(L): 8201572: Improve Metaspace Statistics

Thomas Stüfe thomas.stuefe at gmail.com
Tue Apr 24 14:35:01 UTC 2018


Hi Andrew,

On Tue, Apr 24, 2018 at 3:01 PM, Andrew Dinn <adinn at redhat.com> wrote:
> On 23/04/18 17:32, Thomas Stüfe wrote:
>> 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/
>
> That's a really nice piece of work. Thomas.

Thank you!

>
> As regards the actual code, I only found one questionable change in
> ostream.cpp:
>
>  336   assert(scale == 1 || scale == BytesPerWord || scale == K || scale
> == M || scale == G, "Invalid scale");
>  337   // Special case: printing wordsize should only be done with
> word-sized values
>        . . .
>  343   if (scale == 1) {
>  344     print("%*" PRIuPTR " bytes", width, byte_size);
>  345   } else if (scale == sizeof(MetaWord)) {
>  346     print("%*" PRIuPTR " words", width, byte_size / sizeof(MetaWord));
>
> Why the sudden switch to use sizeof(MetaWord) instead of BytesPerWord?
> It is correct but jars somewhat.

Oh, this was just an oversight. Thanks for spotting, I will fix this.

>
> As for the more general issues:
>
> 1) The code clean-ups are well worth having.
>
> 2) I think all the new functionality is very valuable.
>
> 3) I am happy that this change retains the previously available stats
> reports, albeit in a closely similar format. The few changes to output
> formats may break some users' tooling. However, I think that is more
> than outweighed by the advantages of having much better and clearer
> reporting of the info and the ability to customize reports to include
> more and different data.

I'm happy you see it that way. I tried to strike a balance between
improvement and not causing too much disruption.

The only places where output formats seriously changed are the logging
in case of OOM and Zhengyu's NMT-Metaspace printout.

For the former, I reduced reporting to be safer: before we used to
walk the CLDG and took the expand lock. Now I use the safer
print_basic_report(). I am quite sure the information given by that
function are sufficient and actually I am not sure many people even
knew about this logging output.

As for -XX:+PrintNMTStatistics, I tried to keep the essence of what
Zhengyu did - report total usage plus usage per classloader. It is a
bit more verbose now, and he will have to chime in to say if it is
okay for him. Also, I just found that we reported anonymous loader
usage separately, and this got lost. I will re-add that feature.

>
> 4) I am rather agnostic regarding the use of namespaces, especially when
> used minimally -- as you have done -- simply to distinguish internals
> from externals. So, you can have whatever colour bike-shed you like as
> far as I am concerned. I don't know if there is any general project
> policy regarding their use (or, rather not using them). If so then
> someone else will have to holler.
>

I am sure they will.

> So, in summary, that's an ok from me.

Great, thanks a lot for the quick review!

I will prepare an updated webrev, containing the few changes mentioned
above and also fixing Windows precompiled build which my first webrev
broke (funny to break the precompiled build for a change :)

Best Regards, Thomas

>
> 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


More information about the hotspot-runtime-dev mailing list