Code review request: three native memory tracking related bugs (original) (raw)

Thomas Stüfe thomas.stuefe at gmail.com
Sun Jul 15 07:35:54 PDT 2012

On Sat, Jul 14, 2012 at 8:57 AM, David Holmes <david.holmes at oracle.com> 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/viewbug.do?bugid=7181989 Webrev: http://cr.openjdk.java.net/~zgu/7181989/webrev.00/ We try to assert Thread's stack base to ensure that Thread::recordstackbaseandsize() 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 stackbase(). 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::recordvirtualmemoryrelease for a thread that actually started, and for that you can simplify the check to: if (osthr != NULL && osthr->getstate() > INITIALIZED) and so the whole thing can be expressed simply as: // Update NMT on Thread destruction, but only if the Thread // actually started OSThread* osthr = osthread(); if (osthr != NULL && osthr->getstate() > INITIALIZED) { MemTracker::recordvirtualmemoryrelease( (stackbase() - stacksize()), stacksize(), this); }

I agree with that.

There are a number of fatal()'s e.g. in Linux::current_stack_region() which chould kick in if stack dimension querying fails. However, a guarantee() or fatal() inside Thread::record_stack_base_and_size() would be nice to document that this should never fail.

On a related note, it feels a bit wrong to me to let Thread::record_stack_base_and_size() record stack allocation to the NMT as an unexpected side effect. Also feels asymetric, because recording stack deallocation is done in ~Thread(), as one would expect.

I think I see why you did it - there are 8 or so places where Thread::record_stack_base_and_size() is called. However, the general pattern seems to be that all children of Thread call factored-out functions in their constructors, and I also would do this for NMT stack recording, e.g.:

e.g. void WatcherThread::run() { .... this->record_stack_base_and_size(); this->record_stack_with_NMT(); .... }

Kind Regards, Thomas Stüfe SAP Germany

More information about the hotspot-dev mailing list