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

[Zhengyu Gu]

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.

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