RFR: 8073545 - Use shorter and more descriptive names for GC worker threads

Thu Mar 5 16:19:23 UTC 2015

Looks good!
/Jesper

David Lindholm skrev den 5/3/15 15:59:
> Hi,
>
> Thanks. New webrev available at:
> http://cr.openjdk.java.net/~mgerdin/david/8073545/webrev.01/
>
>
> Regards,
> David
>
> On 2015-03-05 14:41, Erik Helin wrote:
>> On 2015-03-05, David Lindholm wrote:
>>> Hi,
>>>
>>> Thanks for your input. To me, threads and tasks are different things. A task
>>> is something a thread can do for a while, but when it is finished with that
>>> task it can pick something else to do (another task). This view seems to be
>>> reflected in gcTaskThread.cpp (the thread for running tasks in
>>> parallelScavenge). After creation, it runs different tasks managed by
>>> gcTaskManager. These tasks are called names ending with "task".
>>>
>>> But I agree that in most cases the word Thread is unnecessary (this was why
>>> I called the Marker Thread "Marker"). But I don't have anything against
>>> using up all the 15 chars (we should not shorten the name unnecessarily).
>>>
>>> My new suggestion:
>>>
>>> "CMS Thread#xxx"
>>> "CMS Main Thread"
>>> "G1 Refine#xxx"
>>> "G1 Marker#xxx"
>>> "G1 Main Marker"
>>> "G1 StrDedup#xxx"
>>> "ParGC Thread#xx"
>>> "GC Thread#xxx"
>> These names looks good to me. I also had a look at the patch and
>> noticed:
>> -  set_name("GC task thread#%d (ParallelGC)", which);
>> +//  set_name("GC task thread#%d (ParallelGC)", which);
>>
>> Could you please remove the line you commented out?
>>
>> Thanks,
>> Erik
>>
>>> This way our threads naming scheme will be aligned with the rest of the JVM.
>>> One could argue if '#' is necessary. We could leave it out or print '-'
>>> instead. The CompilerThreads leave them out.
>>>
>>>
>>> Thanks,
>>> David
>>>
>>> On 2015-03-04 16:24, Bengt Rutisson wrote:
>>>> Hi David and everyone,
>>>>
>>>> Naming is the most difficult part of computer science so I doubt that
>>>> there is an easy answer here. Given that the names are for debugging I
>>>> guess it is not too important exactly what the names are. More important
>>>> is that they are unique. I also think that there is really no need to have
>>>> "thread" in the name since it is pretty obvious that it is a thread.
>>>>
>>>> So, my suggestion would be to skip "thread" and to use CamelCase rather
>>>> than using spaces to save some characters.
>>>>
>>>> I don't have strong opinions on this, but here's what I'd suggest:
>>>>
>>>> -      _conc_workers = new YieldingFlexibleWorkGang("Parallel CMS
>>>> Threads",
>>>> +      _conc_workers = new YieldingFlexibleWorkGang("CMS Threads",
>>>>
>>>> "CMSTask"
>>>>
>>>>
>>>>
>>>> -  set_name("Concurrent Mark-Sweep GC Thread");
>>>> +  set_name("CMS Main Thread");
>>>>
>>>> "CMSMain"
>>>>
>>>>
>>>>
>>>> -  set_name("G1 Concurrent Refinement Thread#%d", worker_id);
>>>> +  set_name("G1 ConRefine#%d", worker_id);
>>>>
>>>> "G1ConcRefine#%d"
>>>>
>>>>
>>>>
>>>> -  _parallel_workers = new FlexibleWorkGang("G1 Parallel Marking Threads",
>>>> +  _parallel_workers = new FlexibleWorkGang("G1 Markers"
>>>>
>>>> "G1MarkTask"
>>>>
>>>>
>>>>
>>>> -  set_name("G1 Main Concurrent Mark GC Thread");
>>>> +  set_name("G1 Main Marker");
>>>>
>>>> "G1ConcMark"
>>>>
>>>>
>>>>
>>>> -  set_name("String Deduplication Thread");
>>>> +  set_name("StrDedup Thread");
>>>>
>>>> "StringDedup"
>>>>
>>>>
>>>>
>>>> -  set_name("GC task thread#%d (ParallelGC)", which);
>>>> +  set_name("ParGC Thread#%d", which);
>>>>
>>>> "ParGCTask"
>>>>
>>>>
>>>>
>>>> -    _workers = new FlexibleWorkGang("Parallel GC Threads",
>>>> ParallelGCThreads,
>>>> +    _workers = new FlexibleWorkGang("GC Threads", ParallelGCThreads,
>>>>
>>>> "GCParTask"
>>>>
>>>>
>>>> Just my thoughts. And regarding having 3 characters left for the number in
>>>> the thread name I don't know if that is too important. With the above
>>>> suggestions this will be possible in most cases but not all. Personally I
>>>> think that is good enough.
>>>>
>>>> Bengt
>>>>
>>>>
>>>> On 2015-03-04 15:11, David Lindholm wrote:
>>>>> Hi Jesper,
>>>>>
>>>>> On 2015-03-04 14:52, Jesper Wilhelmsson wrote:
>>>>>> Hi David,
>>>>>>
>>>>>> Thanks for fixing this!
>>>>>>
>>>>>> A couple of questions:
>>>>>>
>>>>>> -  set_name("G1 Concurrent Refinement Thread#%d", worker_id);
>>>>>> +  set_name("G1 ConRefine#%d", worker_id);
>>>>>>
>>>>>> Is there any refinement threads that are not concurrent? If not, could
>>>>>> we just call this "G1 Refine#%d" to simplify it slightly and remove an
>>>>>> implementation detail that doesn't need to be exposed? This would also
>>>>>> leave room for three digit numbers in case we have lots of these
>>>>>> threads on some systems.
>>>>> I discussed this with Bengt, and his input was that Concurrent
>>>>> Refinement is a well known concept in G1. I have no real opinion here,
>>>>> I'm fine with both suggestions.
>>>>>
>>>>>> -  _parallel_workers = new FlexibleWorkGang("G1 Parallel Marking
>>>>>> Threads",
>>>>>> +  _parallel_workers = new FlexibleWorkGang("G1 Markers",
>>>>>>
>>>>>> Markers is cute, but could be misunderstood. Can we call it "G1 Mark
>>>>>> Threads" instead?
>>>>> No, it is too long, the three last character with thread number won't
>>>>> fit (#xx).
>>>>>
>>>>>> -  set_name("G1 Main Concurrent Mark GC Thread");
>>>>>> +  set_name("G1 Main Marker");
>>>>>>
>>>>>> Again, "Marker" could be misunderstood. I don't have a good
>>>>>> replacement though.
>>>>> I'm open for suggestions, but I think "G1 Main Marker" works.
>>>>>
>>>>>> -  set_name("GC task thread#%d (ParallelGC)", which);
>>>>>> +  set_name("ParGC Thread#%d", which);
>>>>>>
>>>>>> I don't have a good suggestion for how to make this one character
>>>>>> shorter, but currently there is only room for two digit numbers. Maybe
>>>>>> just "GC Thread#%d". I don't think these threads will exist at the
>>>>>> same time as any other GC threads anyway.
>>>>> With your suggestion these threads would be called the same thing as the
>>>>> threads in sharedHeap. I think it is nice to quickly be able to see that
>>>>> these threads indeed belongs to the ParallelGC.
>>>>>
>>>>>> -  set_name("Gang worker#%d (%s)", id, gang->name());
>>>>>> +  set_name("%s#%d", gang->name(), id);
>>>>>>
>>>>>> Is there any limitation on the length of the name()? If it's too long
>>>>>> the number won't show. Can we add an assert to make sure it isn't too
>>>>>> long?
>>>>> I have gone through our current GangWorkers, and they fit. If you want I
>>>>> can add an assert for <= 12 characters. OTOH it is not the end of the
>>>>> world if we don't see the whole number in the debugger.
>>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> Thanks,
>>>>>> /Jesper
>>>>>>
>>>>>>
>>>>>> David Lindholm skrev den 4/3/15 13:48:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review this small fix which changes the names of the GC
>>>>>>> threads to be
>>>>>>> shorter and more descriptive. There is a limit on 16 characters
>>>>>>> including the
>>>>>>> terminating null byte for this name, since pthread_set_name_np() is
>>>>>>> used. This
>>>>>>> change will make it easier to debug, as these names shows up in the
>>>>>>> debugger.
>>>>>>>
>>>>>>> Webrev: http://cr.openjdk.java.net/~brutisso/8073545/webrev.00/
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8073545
>>>>>>>
>>>>>>> Testing: Passed JPRT.
>>>>>>>
>>>>>>>
>>>>>>> Regards,
>>>>>>> David
>