RFR: 8246134: ZGC: Restructure hs_err sections

Per Liden per.liden at oracle.com
Mon Jun 1 14:00:19 UTC 2020


Looks good!

/Per

> On 1 Jun 2020, at 15:18, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
> 
> Updated webrev after Per's comments:
> https://cr.openjdk.java.net/~stefank/8246134/webrev.02
> 
> StefanK
> 
>> On 2020-06-01 10:24, Per Liden wrote:
>>> On 5/29/20 12:13 PM, Stefan Karlsson wrote:
>>> Hi all,
>>> 
>>> Please review this small patch to restructure and cleanup the information ZGC prints to hs_err files (and jcmd VM.info).
>>> 
>>> https://cr.openjdk.java.net/~stefank/8246134/webrev.01/
>>> https://bugs.openjdk.java.net/browse/JDK-8246134
>>> 
>>> The patch:
>>> - Moves the Page Table dumping later, to make it easier to find the other sections
>>> - Pretty print some info
>>> - Add barrier set print (mostly to get rid of awkward double new lines)
>>> - Update titles and cleanup newlines
>> 
>> Looks good. Some nits:
>> 
>> 1) How about we move z_global_phase_string() to zGlobal.hpp/cpp and call it e.g. ZGlobalPhaseToString()?
>> 
>> 
>> 2) I find code like this unnecessarily hard to read:
>> 
>> +  switch (ZGlobalPhase) {
>> +  case ZPhaseMark: return "Mark";
>> +  case ZPhaseMarkCompleted: return "MarkCompleted";
>> +  case ZPhaseRelocate: return "Relocate";
>> +  default: assert(false, "Unknown ZGlobalPhase"); return "Unknown";
>> 
>> How about:
>> 
>> switch (ZGlobalPhase) {
>> case ZPhaseMark:
>>   return "Mark";
>> 
>> case ZPhaseMarkCompleted:
>>   return "MarkCompleted";
>> 
>> case ZPhaseRelocate:
>>   return "Relocate";
>> 
>> default:
>>   assert(false, "Unknown ZGlobalPhase");
>>   return "Unknown";
>> }
>> 
>> 
>> 3) I see it was like this before your change, but how about removing the extra space on all print_cr-lines, for example:
>> 
>>  317   st->print_cr( "ZGC Globals:");
>> 
>> to:
>> 
>>  317   st->print_cr("ZGC Globals:");
>> 
>> It also looks like the argument indentation here is off by one:
>> 
>>  320   st->print_cr( " Offset Max:        " SIZE_FORMAT "%s (" PTR_FORMAT ")",
>>  321               byte_size_in_exact_unit(ZAddressOffsetMax),
>>  322               exact_unit_for_byte_size(ZAddressOffsetMax),
>>  323               ZAddressOffsetMax);
>>  321               byte_size_in_exact_unit(ZAddressOffsetMax),
>>  322               exact_unit_for_byte_size(ZAddressOffsetMax),
>>  323               ZAddressOffsetMax);
>> 
>> cheers,
>> Per
>> 
>>> 
>>> Thanks,
>>> StefanK
> 




More information about the hotspot-gc-dev mailing list