Code review request: three native memory tracking related bugs

Zhengyu Gu at
Mon Jul 16 07:39:47 PDT 2012

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:
>> Webrev:
>> 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:
>> Webrev:
>> 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
>> CR
>> Webrev:
>> - 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.



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

More information about the hotspot-dev mailing list