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

[David Holmes]

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