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

[Thomas Stüfe]

Still looks good to me. Played with it and the numbers make sense.

Cheers, Thomas

On Tue, Jul 2, 2019 at 11:19 PM Tony Printezis <tprintezis at twitter.com>
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.
>
> 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
>
>
>