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

[Christian Törnqvist]

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. 

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