RFR(S): 8223575: add subspace transitions to gc+metaspace=info log lines

[Tony Printezis]

Thomas and Thomas,

Latest webrev here:

http://cr.openjdk.java.net/~tonyp/8223575/webrev.4/

I reverted the method name back to print_metaspace_change(). I do have an
action item to add a test for this, which I will do on a separate CR, if
that’s OK.

Tony


—————
Tony Printezis | @TonyPrintezis | tprintezis at twitter.com


On July 2, 2019 at 5:13:05 PM, Tony Printezis (tprintezis at twitter.com)
wrote:

Hi Thomas,

Thanks for taking a look. Please see inline.


—————
Tony Printezis | @TonyPrintezis | tprintezis at twitter.com


On July 2, 2019 at 6:34:16 AM, Thomas Schatzl (thomas.schatzl at oracle.com)
wrote:

Hi,

On Wed, 2019-06-05 at 13:25 -0400, Tony Printezis wrote:
> Thanks Thomas, still need to get clarification re: adding Twitter
> copyright notice to the new files. I’ll report back when I have an
> answer.
>

Some minor nits:

- historically method that print log messages are prefixed with
"print_" and not with "log_" like "log_metaspace_change" (which
originally has also been called "print_metaspace_change"). There does
not seem to be lots of reason to change this given that this method
appears a few times in the vicinity of "print_" methods.

For the sake of uniformity I would prefer if that would be kept; I am
open to changing all of these at once (later). Just pointing out.


OK, fair enough. Thomas (Stuefe) suggest to change it to log_ to be more
consistent with what it actually does and I thought it was a good
suggestion. But I can change it back. Worth doing a global rename of the
print_ methods to log_ (as a separate change as you suggested)?



- just a thought: the units of the output for metaspace logging are
hardcoded with "K" (and previously were). It might be useful to adjust
the unit as needed like other similar output.
That's probably one for another CR though.


I could do it as a follow-up (see below, though). I’m planning a couple of
follow-up changes, to add sub-space size transitions for the young gen for
parallel / serial / cms (as the log only shows the size transition of the
young gen as a whole). I’m planning to re-use the HEAP_CHANGE_FORMAT macros
for those changes too (which is why I put them in a shared file). Once
they’re all done, we can switch them to using the other format, if you want.

But, I have to say, I’m OK with leaving them as K. I almost never look at
logs these days and I only look at charts / stats generated from the logs.
So, the more granularity the log has, the more accurate the data is (IMHO).



- it would be nice to have a test case for this, just checking the
general format of the output for with/without coops.


Can I do it as a follow-up (so I can parallelize the next few changes)?



[... snipped long text...]
> > > > --
> > > >
> > > > I am not sure if this needs a CSR and/or a Release Note since
> > > > we change the format of the Metaspace entgc log entries and
> > > > that may confuse people. If it does, I can review the CSR for
> > > > you and hopefully it goes fast. I wait for the second reviewers
> > > > opinion on this (preferably someone from Oracles GC group).
> > >
> > > I have no idea if a CSR is needed. Was there a CSR for the
> > > -Xlog:safepoint* changes? I’ll assume one is not needed unless
> > > I’m told otherwise. ;-)

Log messages are not part of the end user interface and can be changed
as needed (but should not randomly).
Potentially this change is worth mentioning in a release note though if
you want to get word out.


Thanks for the explanation! Yes, you’re right that we should not change log
format for no reason. But, in this case, this change does add a lot of
extra (and very helpful, IMHO) information. So, it’s worth it. As I said,
I’m planning a couple of related changes (which I’ll finalize as soon as
this is done). We can add a note to the release notes that covers all of
them.



Hopefully I did not overlook some important question in the very long
email thread so far.


The last point is the one we were unsure about! I think we’re all set. I’ll
post a new webrev shortly.


Tony




Thanks,
Thomas