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

[Zhengyu Gu]

So guess the only way is to move up the lock to MemTracker class. The snapshot is singleton anyway, the change should be straight forward.

Thanks,

-Zhengyu

Sent from my iPhone

On Jan 13, 2013, at 7:09 PM, David Holmes <david.holmes at oracle.com> wrote:

> On 12/01/2013 12:16 AM, Zhengyu Gu wrote:
>> 
>> 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;
> 
> You can not delete the object that is being waited upon if the object used for waiting is going to be deallocated by the delete! This whole waiting aspect seems flawed to me.
> 
> David
> -----
> 
>> 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
>>>>>