RFR (M) 8005012: Add WB APIs to better support NMT testing

[David Holmes]

Hi Christian,

On 25/01/2013 4:25 AM, Christian Törnqvist wrote:
> Hi David,
>
> My suggestion is that I open a bug to clean up the snapshot->wait() thing in both MemTrackWorker::run() and MemTracker::wbtest_wait_for_data_merge() and do this as separate task since this would also change the way NMT works today.
>
> As for this change I think there are two options, I can either just replace the snapshot->wait() call in wbtest_wait_for_data_merge() with os::sleep(), this would make it different from MemTrackWorker::run() but most probably wouldn't break anything. Or I could leave it as is and do the cleanup in both methods at the same time.

The cleanup can wait.

Thanks,
David

> Thanks,
> Christian
>
> -----Original Message-----
> From: Christian Törnqvist
> Sent: den 21 januari 2013 14:03
> To: David Holmes; Zhengyu Gu
> Cc: hotspot-runtime-dev at openjdk.java.net
> Subject: RE: RFR (M) 8005012: Add WB APIs to better support NMT testing
>
> Hi David,
>
>> Then add a comment here:
>>
>> 577 bool MemTracker::wbtest_wait_for_data_merge() {
>> +     // NMT can't be shutdown while we hold this lock
>> 578   MutexLockerEx lock(_query_lock, true);
>
> I've added the comment
>
>> but I don't see where any notify/notifyAll is now taking place ???
>
> There is no notify/notifyAll on the snapshot _lock, the only time it's used is by MemTrackWorker::run() and MemTracker::wbtest_wait_for_data_merge() which both call wait(1000) on it and could be replace by a call to os::sleep() if waiting 1000ms is the only thing we're after here, Zhengyu might know? Not sure if it's in the scope of this change to correct this though?
>
> Thanks,
> Christian
>
> -----Original Message-----
> From: David Holmes
> Sent: den 15 januari 2013 02:05
> To: Zhengyu Gu
> Cc: Christian Törnqvist; hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR (M) 8005012: Add WB APIs to better support NMT testing
>
> On 15/01/2013 12:56 AM, Zhengyu Gu wrote:
>> On 1/14/2013 9:27 AM, Christian Törnqvist wrote:
>>>> Yes, have notify_all in destructor is a really bad idea, but you do
>>>> need notify_all before delete memSnapshot, since
>>>> MemTracker::wbtest_wait_for_data_merge() can still wait on snapshot ...
>>> But the only place where we delete snapshot is in
>>> MemTracker::final_shutdown() which acquires query_lock before
>>> deleting snapshot so wbtest_wait_for_data_merge() will not be able to
>>> wait on snapshot at this point.
>>>
>> You are right. Look good to me.
>
> Then add a comment here:
>
> 577 bool MemTracker::wbtest_wait_for_data_merge() {
> +     // NMT can't be shutdown while we hold this lock
> 578   MutexLockerEx lock(_query_lock, true);
>
> but I don't see where any notify/notifyAll is now taking place ???
>
> David
>
>> Thanks,
>>
>> -Zhengyu
>>
>>> Thanks,
>>> Christian
>>>
>>> -----Original Message-----
>>> From: Zhengyu Gu
>>> Sent: den 11 januari 2013 15:16
>>> To: David Holmes
>>> Cc: Christian Törnqvist; hotspot-runtime-dev at openjdk.java.net
>>> Subject: Re: RFR (M) 8005012: Add WB APIs to better support NMT
>>> testing
>>>
>>>
>>> Please see comments inline.
>>>
>>> On 1/11/2013 1:48 AM, David Holmes wrote:
>>>> On 10/01/2013 8:52 PM, Christian Törnqvist wrote:
>>>>> Hi David,
>>>>>
>>>>>> This method blocks while holding the query_lock - is that intended?
>>>>> Yes, this will prevent NMT from shutting down and freeing the data
>>>>> structures we're using.
>>>> Ok. Of course this screams deadlock potential to me :)
>>>>
>>> No, I don't think that there is deadlock potential. _query_lock is
>>> used to serialize NMT queries, there is no NMT related lock can be
>>> acquired before it.
>>>
>>>>>> Aside: the MemSnapshot::wait method is troubling me. What are you
>>>>>> waiting upon ie what state condition? The notification is done in
>>>>>> the destructor but how can the destructor be called if someone is
>>>>>> calling
>>>>>> wait() upon the object's lock ??? It means you are destroying an
>>>>>> object that is still in use, including the lock that is being
>>>>>> waited upon!!!
>>> Yes, have notify_all in destructor is a really bad idea, but you do
>>> need notify_all before delete memSnapshot, since
>>> MemTracker::wbtest_wait_for_data_merge() can still wait on snapshot ...
>>> How about in MemTracker::final_shutdown()
>>>
>>> _snapshot->notify_all_waiters();
>>> delete _snapshot;
>>>
>>> Thanks,
>>>
>>> -Zhengyu
>>>
>>>>> This was obviously incorrect, thanks for catching this. I've
>>>>> removed it and done some small other cleanups, a new webrev can be
>>>>> found at http://cr.openjdk.java.net/~ehelin/8005012/webrev.01/
>>>> I don't see memSnapshot in the new webrev - was this all new stuff
>>>> that is now gone?
>>>>
>>>> David
>>>>
>>>>
>>>>> Thanks,
>>>>> Christian
>>>>>
>>>>> -----Original Message-----
>>>>> From: David Holmes
>>>>> Sent: den 19 december 2012 00:21
>>>>> To: Christian Törnqvist
>>>>> Cc: hotspot-runtime-dev at openjdk.java.net; Zhengyu Gu
>>>>> Subject: Re: RFR (M) 8005012: Add WB APIs to better support NMT
>>>>> testing
>>>>>
>>>>> On 19/12/2012 12:10 AM, Christian Törnqvist wrote:
>>>>>> Hi everyone,
>>>>>>
>>>>>> This change is about adding three new WB APIs to better support
>>>>>> NMT testing. The changes are:
>>>>>>
>>>>>> ·A Test memory type, used by WB API's NMTAllocTest and
>>>>>> NMTFreeTestMemory
>>>>>>
>>>>>> ·Added a WaitForDataMerge WB API to be able to block until the
>>>>>> current generation has been merged and is visible, most of this
>>>>>> work was done by Zhengyu Gu.
>>>>> This method blocks while holding the query_lock - is that intended?
>>>>>
>>>>> Aside: the MemSnapshot::wait method is troubling me. What are you
>>>>> waiting upon ie what state condition? The notification is done in
>>>>> the destructor but how can the destructor be called if someone is
>>>>> calling
>>>>> wait() upon the object's lock ??? It means you are destroying an
>>>>> object that is still in use, including the lock that is being
>>>>> waited upon!!!
>>>>>
>>>>> David
>>>>> -----
>>>>>
>>>>>> Webrev can be found at:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~ehelin/8005012/webrev.00/
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Christian
>>>>>>