RFR(L): 8201572: Improve Metaspace Statistics

[Andrew Dinn]

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.

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.

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.

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.

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

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