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

[Tony Printezis]

Hi Thomas,

I created JDK-8227179: "Test for new gc+metaspace=info output format”
and I’ll start working on it today.

Tony


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


On July 3, 2019 at 4:29:21 AM, Thomas Schatzl (thomas.schatzl at oracle.com)
wrote:

Hi,

On Tue, 2019-07-02 at 14:19 -0700, Tony Printezis wrote:
> 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.
>

Okay.

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

>From my POV, not :)

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

Fine with me. This has merely been a suggestion.

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

Okay.

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

I am totally fine with changing the log output as necessary. The
release note is optional too.

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

Looks good.

Thomas