RFR: 8138920: Refactor the sampling thread from ConcurrentG1RefineThread

[Derek White]

Hi Per,

More comments below...

On 10/26/15 12:21 PM, Derek White wrote:
> Hi Per,
>
> On 10/26/15 10:56 AM, Per Liden wrote:
>> On 2015-10-26 15:47, Derek White wrote:
>>> Hi Per,
>>>
>>> On 10/26/15 3:09 AM, Per Liden wrote:
>>>> Hi Derek,
>>>>
>>>> On 2015-10-23 19:43, Derek White wrote:
>>>>> RFR 5.
>>>>>
>>>>> This responds to Per's last comments and Thomas's suggestions for 
>>>>> better
>>>>> methods names and comments.
>>>>>
>>>>> Webrev: http://cr.openjdk.java.net/~drwhite/8138920/webrev.05/
>>>>> Webrev 05 vs 04:
>>>>> http://cr.openjdk.java.net/~drwhite/8138920/webrev.05.vs.04/
>>>>>
>>>>> RFE: JDK-8138920 <https://bugs.openjdk.java.net/browse/JDK-8138920>
>>>>> Refactor the sampling thread from ConcurrentG1RefineThread
>>>>
>>>> The patch looks good, but it looks like you forgot to address the
>>>> following two comments.
>>>
>>> Ahh, I got the Monitor name, but forgot the thread name. Thanks Per!
>>
>> Derek, note that my first comment below about _monitor is not about 
>> its name, but about how it's instantiated, i.e. please remove the 
>> dynamic allocation. Fixing the name is also good, but that was not 
>> what my comment was about.
>
> OK. Sorry for rushing past this.
So Monitor is declared as a CHeapObj. Is it possible to avoid dynamic 
allocation here? Are there examples of this anywhere?

  - Derek

>>
>> cheers,
>> /Per
>>
>>>
>>>   - Derek
>>>>
>>>> [...]
>>>>>>>> g1YoungListSampleThread.cpp
>>>>>>>> ---------------------------
>>>>>>>>
>>>>>>>>   59   _monitor = new Monitor(Mutex::nonleaf,
>>>>>>>>   60                          "Sample thread monitor",
>>>>>>>>   61                          true,
>>>>>>>>   62 Monitor::_safepoint_check_never);
>>>>>>>>
>>>>>>>> It looks like _monitor could be a value member instead of a 
>>>>>>>> pointer,
>>>>>>>> to avoid the extra allocation here.
>>>>>>>>
>>>>>>>>   66   set_name("G1 Sample");
>>>>>>>>
>>>>>>>> This should probably be updated to reflect the new name.
>>>>
>>>> I don't need to see a new webrev, but please address these before
>>>> pushing.
>>>>
>>>> cheers,
>>>> /Per
>>>
>