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

[Thomas Schatzl]

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