RFR (XS) 8222818: NMT summary could show the GC in use

[Stefan Karlsson]

On 2019-04-24 11:18, David Holmes wrote:
> Hi Stefan,
> 
> On 24/04/2019 6:47 pm, Stefan Karlsson wrote:
>>
>>
>> On 2019-04-24 00:17, David Holmes wrote:
>>> On 24/04/2019 6:50 am, Zhengyu Gu wrote:
>>>>
>>>>
>>>> On 4/23/19 2:53 PM, Eric Caspole wrote:
>>>>> Hi Zhengyu,
>>>>> Hopefully this email comes through in monospace, the alignment is 
>>>>> OK for me:
>>>>>
>>>>>
>>>>> currently:
>>>>>
>>>>> -                        GC (reserved=379056KB, committed=93220KB)
>>>>>                              (malloc=39184KB #2159)
>>>>>                              (mmap: reserved=339872KB, 
>>>>> committed=54036KB)
>>>>>
>>>>>
>>>>> My version:
>>>>>
>>>>>
>>>>> -                GC - g1 gc (reserved=379090KB, committed=93254KB)
>>>>>                              (malloc=39218KB #2194)
>>>>>                              (mmap: reserved=339872KB, 
>>>>> committed=54036KB)
>>>>>
>>>>>
>>>>> so it is aligned going to the left off the parenthesis like the 
>>>>> current version. Is that what you mean? I like the way the GC 
>>>>> stands out like this but it is OK to put it in the parentheses on 
>>>>> the right.
>>>>
>>>> Different GC has different name, it is hard to get them all aligned 
>>>> right, and it does not worth the effort.
>>>
>>> AFAICS The code already "reserves" 26 characters just to print "GC", 
>>> which is right-aligned. So all this does is take some of the 24 
>>> existing spaces and fill them in with the GC name so you end up with:
>>>
>>>                  GC - g1 gc (reserved=379056KB, committed=93220KB)
>>>                             (malloc=39184KB #2159)
>>>                             (mmap: reserved=339872KB, committed=54036KB)
>>>
>>> or
>>>          GC - shenandoah gc (reserved=379056KB, committed=93220KB)
>>>                             (malloc=39184KB #2159)
>>>                             (mmap: reserved=339872KB, committed=54036KB)
>>>
>>> etc. Only nit is that 26 seems to small for "concurrent mark sweep 
>>> gc". Also the alignment of 26 could be specified dynamically based on 
>>> the length of the hs_err_name() string if needed.
>>
>>
>> FYI, the name of the function hs_err_name() was chosen to deter people 
>> from using it in other places. Maybe it's time to add another function 
>> in GCConfig that return better names? Maybe use names that match our 
>> -XX:Use<Name>GC flags?
> 
> I was thinking more simply that we might rename hs_err_name() to name() 
> and make the strings a little nicer e.g. "G1 GC" instead of "g1 gc". But 
> if you want to add more general GC functionality here that's up to you :)

Either way works for me.

We tried to change the name before, but it was met with resistance:
https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2018-April/021937.html

StefanK

> 
> Cheers,
> David
> 
>> If you would find that useful, I've created a patch that exposes three 
>> new functions that return the flag names, or parts of it:
>> http://cr.openjdk.java.net/~stefank/8222818/gcFlagNames/webrev.01/
>>
>> GCConfig::flag_name() gives:
>>   UseConcMarkSweepGC
>>   UseEpsilonGC
>>   UseG1GC
>>   UseParallelGC
>>   UseSerialGC
>>   UseShenandoahGC
>>   UseZGC
>>
>> GCConfig::flag_name_no_use() gives:
>>   ConcMarkSweepGC
>>   EpsilonGC
>>   G1GC
>>   ParallelGC
>>   SerialGC
>>   ShenandoahGC
>>   ZGC
>>
>> GCConfig::flag_name_no_use_no_gc() gives:
>>   ConcMarkSweep
>>   Epsilon
>>   G1
>>   Parallel
>>   Serial
>>   Shenandoah
>>   Z
>>
>> The universe.cpp changes, are only temporary changes to test the output.
>>
>> StefanK
>>
>>>
>>> David
>>> -----
>>>
>>>>
>>>> So, my suggestion is to place GC name inside parentheses, and you 
>>>> don't have to deal with indents to the left.
>>>>
>>>> e.g.
>>>>
>>>>
>>>> -                     GC (reserved=379056KB, committed=93220KB by g1 
>>>> gc)
>>>>                            (malloc=39184KB #2159)
>>>>                            (mmap: reserved=339872KB, committed=54036KB)
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> -Zhengyu
>>>>
>>>>>
>>>>> Thanks,
>>>>> Eric
>>>>>
>>>>>
>>>>>
>>>>> On 4/22/19 21:57, Zhengyu Gu wrote:
>>>>>>
>>>>>>
>>>>>> On 4/22/19 8:19 PM, David Holmes wrote:
>>>>>>> Hi Eric,
>>>>>>>
>>>>>>> On 23/04/2019 8:13 am, Eric Caspole wrote:
>>>>>>>> Hi, could I have reviews and any opinions on this little change 
>>>>>>>> to show the GC name in the NMT output, as this helps us to more 
>>>>>>>> easily triage performance data.
>>>>>>>
>>>>>>> The idea seems fine.
>>>>>>
>>>>>>>
>>>>>>> For the implementation wouldn't it be simpler to do something like:
>>>>>>>
>>>>>>> if (flag == mtGC) {
>>>>>>>    out->print("%s - %s (", NMTUtil::flag_to_name(flag),
>>>>>>>                            GCConfig::hs_err_name());
>>>>>>> } else {
>>>>>>>    out->print("-%26s (", NMTUtil::flag_to_name(flag));
>>>>>>> }
>>>>>>>
>>>>>> Yes, this is simpler.
>>>>>>
>>>>>> I don't like where the name is placed, it screws up section 
>>>>>> alignments. I would prefer to place name inside parenthesis. e.g.
>>>>>>
>>>>>> - GC (g1 gc reserved=379056KB, committed=93220KB)
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> -Zhengyu
>>>>>>
>>>>>>> and skip the need for a local buffer and snprintf?
>>>>>>>
>>>>>>> Aside: it's probably used in enough different contexts that 
>>>>>>> GCConfig::hs_err_name should be renamed.
>>>>>>>
>>>>>>> Also if the VM terminates during initialization is it possible 
>>>>>>> for this code to be executed before the GCConfig has been setup? 
>>>>>>> And if so how will it behave?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>>>> This passed tier 1 and 2.
>>>>>>>> Thanks,
>>>>>>>> Eric
>>>>>>>>
>>>>>>>>
>>>>>>>> JBS:
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8222818
>>>>>>>>
>>>>>>>> webrev:
>>>>>>>> http://cr.openjdk.java.net/~ecaspole/JDK-8222818/02/webrev/