RFR: 8138920: Refactor the sampling thread from ConcurrentG1RefineThread

[Per Liden]

Hi Derek,

On 2015-10-26 22:01, Derek White wrote:
> 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.

Using dynamic allocation when it's not needed is just sloppy, IMHO.

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

I think you're overstating the problem here. In this case the sample 
thread (or rather the ConcurrentG1Refine instance which owns the sample 
thread) is never destroyed so you wouldn't be changing the life time. On 
the other hand, if the sample thread had been destroyed, but not the 
_monitor, then we have a memory leak which should be fixed.

cheers,
/Per

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