RFR: 8138920: Refactor the sampling thread from ConcurrentG1RefineThread

[Per Liden]

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