Code review request: three native memory tracking related bugs

[David Holmes]

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);
}

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

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

Cheers,
David

>
> Thanks,
>
> -Zhengyu
>