RFR: 8138920: Refactor the sampling thread from ConcurrentG1RefineThread

Tue Oct 27 15:30:41 UTC 2015

Thanks Per!

New issue: JDK-8140600 
<https://bugs.openjdk.java.net/browse/JDK-8140600> Convert unnecessarily 
malloc'd Monitors to value members

Kim already offered to sponsor, so I'll send it his way (he can more 
easily hunt me down if any issues arise).

  - Derek

On 10/27/15 11:13 AM, Per Liden wrote:
> Hi Derek,
>
> On 2015-10-27 14:31, Oracle wrote:
>> Hi Per,
>>
>>> On Oct 27, 2015, at 7:22 AM, Per Liden <per.liden at oracle.com> wrote:
>>>
>>> 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
>>
>> Agreed. This is one of many such pre-existing sloppy allocations. I'm 
>> happy to dedicate my time to cleaning these up once I'm certain it's 
>> safe to do so.
>>
>> In the meantime, Kim has been waiting for the cleanup of the sampling 
>> thread since Thursday.
>>
>>>> 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.
>>
>> The sampling thread ownership will soon be removed from 
>> ConcurrentG1Refine, so we can't rely on that. And the dangling 
>> Monitor idiom is so frequent in our code that I'm not sure that it's 
>> unintentional.
>>
>> I agree with you that this should be cleaned up, but I think any fix 
>> involving Monitor lifetime changes will deserve more consideration*** 
>> and review, and that the fix should be applied to more cases across 
>> our code.
>>
>> Meanwhile I'd like to put this code to bed. Are you ok with 
>> separating these issue to s new CR?
>
> I'm ok with that. Let's get this change pushed. I can sponsor it, just 
> let me know where I can find the final patch.
>
> cheers,
> /Per
>
>>
>> Thanks for all your work reviewing this change!
>>
>> - Derek
>>
>>
>> *** I want to see if there's some way to assert that a Monitor has no 
>> waiters before deleting it, for example.
>>
>>
>>> 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
>>>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20151027/1bc96174/attachment.htm>