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

[David Holmes]

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

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/