RFR: 8138920: Refactor the sampling thread from ConcurrentG1RefineThread

[Derek White]

Hi Per,

Even more on the monitor allocation issue below...

  On 10/26/15 12:25 PM, Derek White wrote:
> 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

After talking with Kim about CHeapObj, it sounds like using a 
CHeapObj-typed object as a value object is allowed, although there 
wasn't unanamous agreement that is was a good idea.

After more detective work, it looks like parallel GC as well as 
MutexGangTaskDispatcher do destroy a dynamically allocated Monitor. And 
it looks like SurrogateLockerThread is the one case that uses a Monitor 
as a value member.

But I'm more concerned about changing the lifetime of the Monitor 
object. This may be preventing dead-lock on shutdown - Some thread A is 
waiting on a monitor owned by thread B, and thread B's destructor is 
called, destroying the Monitor being waited on.

I'd rather not tack on this change at this point. If turning dynamically 
allocated Monitors into value members is a good idea, I can add it to 
the technical debt list. But it's not a risk-free change, so we'll need 
to come up with some schemes for validation and testing.

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