RFR: 8252141: Rename G1YoungRemSetSamplingThread to better reflect its purpose

[Thomas Schatzl]

Hi,

On 01.09.20 12:50, stefan.johansson at oracle.com wrote:
> Hi Thomas,
> 
> On 2020-08-31 15:20, Thomas Schatzl wrote:
>> Hi,
>>
>> On 26.08.20 09:18, stefan.johansson at oracle.com wrote:
>>> Hi Kim,
>>>
>>> Thanks for reviewing.
>>>
>>> On 2020-08-25 04:44, Kim Barrett wrote:
>>>>> On Aug 24, 2020, at 5:36 PM, stefan.johansson at oracle.com wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> Please review this rename of the G1 Young RemSet Sampling thread.
>>>>> [...]
>>> Nice catch, updated in:
>>> Full: http://cr.openjdk.java.net/~sjohanss/8252141/01/
>>> Inc: http://cr.openjdk.java.net/~sjohanss/8252141/00-01/
>>>
>>> I also filed a issue to take care of the name at some point:
>>> https://bugs.openjdk.java.net/browse/JDK-8252358
>>
>> Thanks.
>>
> 
> Thanks for reviewing.
> 
>> Some comments:
>>
>> - g1RemSetSummary.cpp:330:
>>
>>    out->print_cr("  Concurrent sampling threads times (s)");
>>    out->print_cr("         %5.2f", service_thread_vtime());
>>
>> should probably be "Service thread time (s)".
>>
> 
> I was thinking about changing this but wasn't sure, since this is for 
> the rem-sets. I think going forward maybe we want to change how we 
> measure the time to only account the actual time spent on the remsets. 
> Filed an issue for this:
> https://bugs.openjdk.java.net/browse/JDK-8252645
> 

Okay.

> In the mean time I updated the code per your request.
> 
>> - if you are bored, some copyright dates need updates
>>
> I'm not bored, but since you called it out :)
> 
>> - pre-existing: maybe the includes in G1ServiceThread.hpp could be 
>> fixed; at least runtime/mutex.hpp should be included for Monitor.
>>
> I added it and os.hpp.
> 
> Updated webrevs:
> Full: http://cr.openjdk.java.net/~sjohanss/8252141/02
> Inc: http://cr.openjdk.java.net/~sjohanss/8252141/01-02

The g1ServiceThread._h_pp file needs to include mutex.hpp as it embeds a 
Monitor object and is needed for compilation, not the _c_pp file.

I do not need a re-review for moving the include declaration. Looks good 
otherwise.

Thanks,
   Thomas