Code review request: three native memory tracking related bugs

[Zhengyu Gu]

Hi David,

On 7/14/2012 2:57 AM, David Holmes wrote:
> On 14/07/2012 5:43 AM, Zhengyu Gu wrote:
>> 7181989: NMT ON: Assertion failure when NMT checks thread's native
>> CR: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7181989
>> Webrev: http://cr.openjdk.java.net/~zgu/7181989/webrev.00/
>>
>> We try to assert Thread's stack base to ensure that
>> Thread::record_stack_base_and_size() to record native stack to NMT, but
>> there is scenario that the thread fails to start, which no native stack
>> is created and should not be asserted.
>
> I'm not really clear on why we need to refer to the assert inside 
> stack_base(). If any started thread fails to setup the stackbase and 
> stacksize then it will likely crash well prior to the Thread 
> destructor. It seems to me that all that is needed here is to only 
> call MemTracker::record_virtual_memory_release for a thread that 
> actually started, and for that you can simplify the check to:
>
> if (os_thr != NULL && os_thr->get_state() > INITIALIZED)
>
> and so the whole thing can be expressed simply as:
>
> // Update NMT on Thread destruction, but only if the Thread
> // actually started
> OSThread* os_thr = osthread();
> if (os_thr != NULL && os_thr->get_state() > INITIALIZED) {
>      MemTracker::record_virtual_memory_release(
>           (stack_base() - stack_size()), stack_size(), this);
> }
Fair enough, since the assertion has yet caught anything, I assume that 
threads are calling record_stack_base_and_size(), so the assertion is 
not necessary.


>
>> 7181986: NMT ON: Assertion failure when running jdi
>> ExpiredRequestDeletionTest
>> CR: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7181986
>> Webrev: http://cr.openjdk.java.net/~zgu/7181986/webrev.00/
>>
>> This is a racing condition when C/C++ runtime exit handler is ran
>> before NMT worker thread exits. The exit handler calls _query_lock's
>> destructor while NMT worker thread is still holding it. The fix is to
>> make _query_lock a heap object, instead of static object, but the
>> drawback is that, it does not seem that _query_lock can be safely 
>> deleted.
>>
>> Also, I reassigned MemSnaphot lock and query lock, so they participate
>> in the deadlock detection logic.
>
> src/share/vm/services/memSnapshot.cpp
>
> We don't use std::nothrow when allocating other VM mutexes, and you 
> are not checking for allocation failures.
>
We do check "_lock == NULL" in out_of_memory(), which is not in the webrev.


> Can't comment on whether the 'level' assigned to the mutexes is 
> appropriate - only testing and time will tell.
>
>> 7182543: NMT ON: Aggregate a few NMT related bugs
>> CRhttp://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7182543
>> Webrev: http://cr.openjdk.java.net/~zgu/7182543/webrev.00/
>>
>> - Fixed generations_in_used calculation
>> - Wait MemRecorder instance count to drop to zero before completely
>> shutdown NMT
>
> So previously the instance_count was only present in debug. Is this 
> likely to be a contention bottleneck?
>
>> - Added assertion for JavaThread in _thread_blocked state.
>
> Is the check for a safepoint needed? I'm assuming this assertion is 
> trying to verify that threads in _thread_blocked don't do 
> allocation/deallocation (though I'm unclear why the constraint 
> exists??). Whether a safepoint is presently active or not seems 
> irrelevant ??
>
The initial assumption was that threads in _thread_blocked don't 
allocate/deallocate memory, but it is not the case ...

Talked to Karen, the conclusion is that, NMT should just use 
ThreadCritical to block the threads in _thread_blocked state.

Thanks,

-Zhengyu

> Cheers,
> David
>
>>
>> Thanks,
>>
>> -Zhengyu
>>